Hi Bingbu,
Thanks for your review! Replies inline...
On 7/4/24 4:03 PM, Bingbu Cao wrote:
diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 3ff390b04e1a..e7aee7e3db5b 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -535,29 +535,51 @@ static void imgu_vb2_stop_streaming(struct vb2_queue *vq)
container_of(vq, struct imgu_video_device, vbq);
int r;
unsigned int pipe;
+ bool stop_streaming = false;
+ /* Verify that the node had been setup with imgu_v4l2_node_setup() */
WARN_ON(!node->enabled);
pipe = node->pipe;
dev_dbg(dev, "Try to stream off node [%u][%u]", pipe, node->id);
- imgu_pipe = &imgu->imgu_pipe[pipe];
- r = v4l2_subdev_call(&imgu_pipe->imgu_sd.subdev, video, s_stream, 0);
- if (r)
- dev_err(&imgu->pci_dev->dev,
- "failed to stop subdev streaming\n");
I guess the subdev streams API can help us on this, but current fix
looks fine for me.
I don't understand what you're referring to, since your comment is in
relation to a block of existing code that I have merely shuffled around.
Could you please elaborate?
+ /*
+ * When the first node of a streaming setup is stopped, the entire
+ * pipeline needs to stop before individual nodes are disabled.
+ * Perform the inverse of the initial setup.
+ *
+ * Part 1 - s_stream on the entire pipeline
stream on or off? it is a little confusing.
I meant that s_stream(off) is called "on the entire pipeline".
Thanks, I'll rephrase this in v2 :)
+ */
mutex_lock(&imgu->streaming_lock);
- /* Was this the first node with streaming disabled? */
if (imgu->streaming) {
/* Yes, really stop streaming now */
dev_dbg(dev, "IMGU streaming is ready to stop");
r = imgu_s_stream(imgu, false);
if (!r)
imgu->streaming = false;
+ stop_streaming = true;
}
-
mutex_unlock(&imgu->streaming_lock);
+ /* Part 2 - s_stream on subdevs
+ *
+ * If we call s_stream multiple times, Linux v6.7's call_s_stream()
+ * WARNs and aborts. Thus, disable all pipes at once, and only once.
+ */
+ if (stop_streaming) {
+ for_each_set_bit(pipe, imgu->css.enabled_pipes,
+ IMGU_MAX_PIPE_NUM) {
+ imgu_pipe = &imgu->imgu_pipe[pipe];
+
+ r = v4l2_subdev_call(&imgu_pipe->imgu_sd.subdev,
+ video, s_stream, 0);
+ if (r)
+ dev_err(&imgu->pci_dev->dev,
+ "failed to stop subdev streaming\n");
+ }
Is it possible to move this loop into 'if (imgu->streaming)' above?
The point of separating the loop from 'if (imgu->streaming)', and why
the stop_streaming variable exists, is to keep the loop out of the
mutex's hot path. This follows the code in imgu_vb2_start_streaming(),
as mentioned in the commit message.
Is it safe to do this work with the mutex taken?
Is there any need to do this work with the mutex taken?
Should the streamoff path really be different from the streamon path?
Does this mean that the streamon path should also have this happen with
the mutex taken?
Thanks,
Max
