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

[Max Staudt <mstaudt@chromium.org>]

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