Fixing IPU3 IMGU warnings due to extraneous calls to s_stream()

Fixing IPU3 IMGU warnings due to extraneous calls to s_stream()

[Max Staudt]

Dear IPU3 driver maintainers,

The Intel IPU3 IMGU driver no longer shuts down cleanly since v6.7,
because vb2 now complains if s_stream() is called multiple times on
the same object:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=009905ec50433259c05f474251000b040098564e

This series attempts to fix this, but needs a review from someone more
intimate with IPU3 and its driver. Could you please have a look at this?


Thanks for your feedback,

Max


 [PATCH v1 1/3] staging: media: ipu3: Drop superfluous check in
 [PATCH v1 2/3] staging: media: ipu3: Return buffers outside of
 [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order

[PATCH v1 1/3] staging: media: ipu3: Drop superfluous check in imgu_vb2_stop_streaming()

[Max Staudt]

The check for imgu_all_nodes_streaming() seems superfluous, since
imgu->streaming can only become true once imgu_all_nodes_streaming()
has been true. Hence, checking for imgu->streaming == true should
imply imgu_all_nodes_streaming(), and therefore suffice.

Signed-off-by: Max Staudt <mstaudt@chromium.org>
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 3df58eb3e882..541556037c42 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -548,7 +548,7 @@ static void imgu_vb2_stop_streaming(struct vb2_queue *vq)
 
 	mutex_lock(&imgu->streaming_lock);
 	/* Was this the first node with streaming disabled? */
-	if (imgu->streaming && imgu_all_nodes_streaming(imgu, node)) {
+	if (imgu->streaming) {
 		/* Yes, really stop streaming now */
 		dev_dbg(dev, "IMGU streaming is ready to stop");
 		r = imgu_s_stream(imgu, false);
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 2/3] staging: media: ipu3: Return buffers outside of needless locking

[Max Staudt]

In imgu_vb2_start_streaming()'s error path, imgu_return_all_buffers()
is outside the streaming_lock and after the call to
video_device_pipeline_stop().

Let's apply the same order in imgu_vb2_stop_streaming() as well.

Signed-off-by: Max Staudt <mstaudt@chromium.org>
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 541556037c42..3ff390b04e1a 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -556,10 +556,10 @@ static void imgu_vb2_stop_streaming(struct vb2_queue *vq)
 			imgu->streaming = false;
 	}
 
-	imgu_return_all_buffers(imgu, node, VB2_BUF_STATE_ERROR);
 	mutex_unlock(&imgu->streaming_lock);
 
 	video_device_pipeline_stop(&node->vdev);
+	imgu_return_all_buffers(imgu, node, VB2_BUF_STATE_ERROR);
 }
 
 /******************** v4l2_ioctl_ops ********************/
-- 
2.45.2.627.g7a2c4fd464-goog

[PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

[Max Staudt]

imgu_vb2_stop_streaming() did not order shutdown items in the inverse
order and count of what imgu_vb2_start_streaming() does. Consequently,
v6.7's new WARN_ON in call_s_stream() started screaming because it was
called multiple times on the entire pipe, yet it should only be called
when the pipe is interrupted by any first node being taken offline.

This reorders streamoff to be the inverse of streamon, and uses
analogous conditions to decide when and how often to call additional
teardown functions.

v4l2_subdev_call(s_stream, 0) remains outside the streaming_lock,
analogously to imgu_vb2_start_streaming().

Signed-off-by: Max Staudt <mstaudt@chromium.org>
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 36 +++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 7 deletions(-)

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");
 
+	/*
+	 * 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
+	 */
 	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");
+		}
+	}
+
+	/* Part 3 - individual node teardown */
 	video_device_pipeline_stop(&node->vdev);
 	imgu_return_all_buffers(imgu, node, VB2_BUF_STATE_ERROR);
 }
-- 
2.45.2.627.g7a2c4fd464-goog

Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

[Bingbu Cao]

Max,

Thanks for your patch.

On 6/20/24 10:45 PM, Max Staudt wrote:
> imgu_vb2_stop_streaming() did not order shutdown items in the inverse
> order and count of what imgu_vb2_start_streaming() does. Consequently,
> v6.7's new WARN_ON in call_s_stream() started screaming because it was
> called multiple times on the entire pipe, yet it should only be called
> when the pipe is interrupted by any first node being taken offline.
> 
> This reorders streamoff to be the inverse of streamon, and uses
> analogous conditions to decide when and how often to call additional
> teardown functions.
> 
> v4l2_subdev_call(s_stream, 0) remains outside the streaming_lock,
> analogously to imgu_vb2_start_streaming().
> 
> Signed-off-by: Max Staudt <mstaudt@chromium.org>
> ---
>  drivers/staging/media/ipu3/ipu3-v4l2.c | 36 +++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> 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.

>  
> +	/*
> +	 * 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.

> +	 */
>  	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?

> +	}
> +
> +	/* Part 3 - individual node teardown */
>  	video_device_pipeline_stop(&node->vdev);
>  	imgu_return_all_buffers(imgu, node, VB2_BUF_STATE_ERROR);
>  }
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

[Max Staudt]

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

Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

[Bingbu Cao]

Max,

On 7/5/24 9:54 AM, Max Staudt wrote:
> 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?

I guess the real problem is the subdev s_stream cannot be called multiple
times, please correct me if I am wrong as I have not touch IPU3 for long
time. :)

You can ignore my previous comment - 's_stream' is fine here.

> 
> 
>>>   +    /*
>>> +     * 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?

Just a nit, stop_stream and s_stream only happen after imgu_s_stream(), so I
think they can be together and no need 'stop_streaming'. I think the mutex
is mainly for imgu_s_stream, subdev stream on/off should work without it.
It depends on you. :)

It'll be better that others who is still working on IPU3 devices can review.

Besides,
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
> 
> 
> 
> Thanks,
> 
> Max
> 
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order of starting

[Max Staudt]

On 7/5/24 1:03 PM, Bingbu Cao wrote:
> I guess the real problem is the subdev s_stream cannot be called multiple
> times, please correct me if I am wrong as I have not touch IPU3 for long
> time. :)
> 
> You can ignore my previous comment - 's_stream' is fine here.

Yes, indeed the multiple calls were what the newer V4L2 versions are 
screaming about, and what made me write this patch...


> Just a nit, stop_stream and s_stream only happen after imgu_s_stream(), so I
> think they can be together and no need 'stop_streaming'. I think the mutex
> is mainly for imgu_s_stream, subdev stream on/off should work without it.
> It depends on you. :)
> 
> It'll be better that others who is still working on IPU3 devices can review.

Okay, let's wait for their feedback then before I send a v2.


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

Appreciated! Let's do a v2, when other Intel folks have had a chance to 
chime in.

Max

Re: Fixing IPU3 IMGU warnings due to extraneous calls to s_stream()

[Ricardo Ribalda]

Tried to get some images using a Google Pixelbook Go


Tested-by: Ricardo Ribalda <ribalda@chromium.org>

On Fri, 21 Jun 2024 at 06:46, Max Staudt <mstaudt@chromium.org> wrote:
>
> Dear IPU3 driver maintainers,
>
> The Intel IPU3 IMGU driver no longer shuts down cleanly since v6.7,
> because vb2 now complains if s_stream() is called multiple times on
> the same object:
>
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=009905ec50433259c05f474251000b040098564e
>
> This series attempts to fix this, but needs a review from someone more
> intimate with IPU3 and its driver. Could you please have a look at this?
>
>
> Thanks for your feedback,
>
> Max
>
>
>  [PATCH v1 1/3] staging: media: ipu3: Drop superfluous check in
>  [PATCH v1 2/3] staging: media: ipu3: Return buffers outside of
>  [PATCH v1 3/3] staging: media: ipu3: Stop streaming in inverse order



-- 
Ricardo Ribalda

Re: Fixing IPU3 IMGU warnings due to extraneous calls to s_stream()

[Sakari Ailus]

Hi Max,

On Thu, Jun 20, 2024 at 11:45:40PM +0900, Max Staudt wrote:
> Dear IPU3 driver maintainers,
> 
> The Intel IPU3 IMGU driver no longer shuts down cleanly since v6.7,
> because vb2 now complains if s_stream() is called multiple times on
> the same object:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=009905ec50433259c05f474251000b040098564e
> 
> This series attempts to fix this, but needs a review from someone more
> intimate with IPU3 and its driver. Could you please have a look at this?

Thanks for the patches. They seem good to me, I've taken them to my tree
(devel branch).

-- 
Kind regards,

Sakari Ailus

Re: Fixing IPU3 IMGU warnings due to extraneous calls to s_stream()

[Max Staudt]

On 8/27/24 3:56 PM, Sakari Ailus wrote:
> Thanks for the patches. They seem good to me, I've taken them to my tree
> (devel branch).

Thanks Sakari for taking the patches, and thanks Ricardo for stepping in 
during my absence - apologies for the silence and not delivering a 
PATCHv2 last year. I'm glad that this was sorted out in the end :)

Max