Re: [PATCHv3 2/4] v4l: vsp1: Refactor video pipeline configuration

[Kieran Bingham <kieran@ksquared.org.uk>]

Hi Laurent,

I've been reworking this series to split things out and adapt for the
comments you've provided, but I have the following queries outstanding:

On 15/12/16 11:50, Kieran Bingham wrote:
> Hi Laurent,
> 
> On 14/12/16 16:30, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On Tuesday 13 Dec 2016 17:59:42 Kieran Bingham wrote:
>>> With multiple inputs through the BRU it is feasible for the streams to
>>> race each other at stream-on.
>>
>> Could you please explain the race condition in the commit message ? The issue 
>> is that multiple VIDIOC_STREAMON calls racing each other could have process 
>> N-1 skipping over the pipeline setup section and then start the pipeline, if 
>> videobuf2 has already enqueued buffers to the driver for process N but not 
>> called the .start_streaming() operation yet.
>>
>>> In the case of the video pipelines, this
>>> can present two serious issues.
>>>
>>>  1) A null-dereference if the pipe->dl is committed at the same time as
>>>     the vsp1_video_setup_pipeline() is processing
>>>
>>>  2) A hardware hang, where a display list is committed without having
>>>     called vsp1_video_setup_pipeline() first
>>>
>>> Along side these race conditions, the work done by
>>> vsp1_video_setup_pipeline() is undone by the re-initialisation during a
>>> suspend resume cycle, and an active pipeline does not attempt to
>>> reconfigure the correct routing and init parameters for the entities.
>>>
>>> To repair all of these issues, we can move the call to a conditional
>>> inside vsp1_video_pipeline_run() and ensure that this can only be called
>>> on the last stream which calls into vsp1_video_start_streaming()
>>>
>>> As a convenient side effect of this, by specifying that the
>>> configuration has been lost during suspend/resume actions - the
>>> vsp1_video_pipeline_run() call can re-initialise pipelines when
>>> necessary thus repairing resume actions for active m2m pipelines.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>>
>>> ---
>>> v3:
>>>  - Move 'flag reset' to be inside the vsp1_reset_wpf() function call
>>>  - Tidy up the wpf->pipe reference for the configured flag
>>>
>>>  drivers/media/platform/vsp1/vsp1_drv.c   |  4 ++++
>>>  drivers/media/platform/vsp1/vsp1_pipe.c  |  1 +
>>>  drivers/media/platform/vsp1/vsp1_pipe.h  |  2 ++
>>>  drivers/media/platform/vsp1/vsp1_video.c | 20 +++++++++-----------
>>>  4 files changed, 16 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
>>> b/drivers/media/platform/vsp1/vsp1_drv.c index 57c713a4e1df..1dc3726c4e83
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>>> @@ -413,6 +413,7 @@ static int vsp1_create_entities(struct vsp1_device
>>> *vsp1)
>>>
>>>  int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned int index)
>>>  {
>>> +	struct vsp1_rwpf *wpf = vsp1->wpf[index];
>>>  	unsigned int timeout;
>>>  	u32 status;
>>>
>>> @@ -429,6 +430,9 @@ int vsp1_reset_wpf(struct vsp1_device *vsp1, unsigned
>>> int index) usleep_range(1000, 2000);
>>>  	}
>>>
>>> +	if (wpf->pipe)
>>> +		wpf->pipe->configured = false;
>>> +
>>>  	if (!timeout) {
>>>  		dev_err(vsp1->dev, "failed to reset wpf.%u\n", index);
>>>  		return -ETIMEDOUT;
>>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
>>> b/drivers/media/platform/vsp1/vsp1_pipe.c index 756ca4ea7668..7ddf862ee403
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
>>> @@ -208,6 +208,7 @@ void vsp1_pipeline_init(struct vsp1_pipeline *pipe)
>>>
>>>  	INIT_LIST_HEAD(&pipe->entities);
>>>  	pipe->state = VSP1_PIPELINE_STOPPED;
>>> +	pipe->configured = false;
>>>  }
>>>
>>>  /* Must be called with the pipe irqlock held. */
>>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
>>> b/drivers/media/platform/vsp1/vsp1_pipe.h index ac4ad2655551..0743b9fcb655
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
>>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
>>> @@ -61,6 +61,7 @@ enum vsp1_pipeline_state {
>>>   * @pipe: the media pipeline
>>>   * @irqlock: protects the pipeline state
>>>   * @state: current state
>>> + * @configured: determines routing configuration active on cell.
>>
>> I'm not sure to understand that. How about "true if the pipeline has been set 
>> up" ? Or maybe "true if the pipeline has been set up for video streaming" as 
>> it only applies to pipelines handled through the V4L2 API ?
> 
> 
> Yes, Reading it now - I have no idea what context I was writing that in.
> I hope it was late and I was tired ... otherwise I have no excuse :D
> 
> 
> 
>>>   * @wq: wait queue to wait for state change completion
>>>   * @frame_end: frame end interrupt handler
>>>   * @lock: protects the pipeline use count and stream count
>>> @@ -86,6 +87,7 @@ struct vsp1_pipeline {
>>>
>>>  	spinlock_t irqlock;
>>>  	enum vsp1_pipeline_state state;
>>> +	bool configured;
>>>  	wait_queue_head_t wq;
>>>
>>>  	void (*frame_end)(struct vsp1_pipeline *pipe);
>>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>>> b/drivers/media/platform/vsp1/vsp1_video.c index 44b687c0b8df..7ff9f4c19ff0
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>>> @@ -388,6 +388,8 @@ static int vsp1_video_setup_pipeline(struct
>>> vsp1_pipeline *pipe) VSP1_ENTITY_PARAMS_INIT);
>>>  	}
>>>
>>> +	pipe->configured = true;
>>> +
>>>  	return 0;
>>>  }
>>>
>>> @@ -411,6 +413,9 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline
>>> *pipe) struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
>>>  	struct vsp1_entity *entity;
>>>
>>> +	if (!pipe->configured)
>>> +		vsp1_video_setup_pipeline(pipe);
>>> +
>>
>> I don't like this much. The vsp1_video_pipeline_run() is called with a 
>> spinlock held. We should avoid operations as time consuming as 
>> vsp1_video_setup_pipeline() here.


I'm wondering where you stand on this now that the measurements have
been taken.

Is 18uS, once per stream start (/resume) still against your desires here?

Reading the comments from, struct vsp1_pipeline

 * @irqlock: protects the pipeline state
 * @lock: protects the pipeline use count and stream count

Doesn't this infer that calling vsp1_video_setup_pipeline, is better run
with the @irqlock held rather than the @lock anyway?


> I'm going to argue my case here, as I thought this was a more elegant
> solution (and I really seem to dislike that poorly protected global
> pipe->dl);
> 
> However I will back down quickly/happily if needed. Especially knowing
> that we are trying to reduce the recalculations of the DL, and I can see
> where those changes will lead us anyway.
> 
> Regardless of the outcome, I felt it was useful (at least to me) to
> measure the call times on these functions to get an understanding, thus
> I have used ftrace to measure/monitor a full run of the vsp-test suite:
> 
>  trace-cmd record \
>         -p function_graph \
>         -l 'vsp1_video_setup_pipeline' \
>         -l 'vsp1_irq_handler' \
>         \
>         ./vsp-tests.sh
> 
> * I had to mark vsp1_video_setup_pipeline() as noinline to be able to
> measure it with ftrace
> 
> A) Call frequency
> 
> Whilst vsp1_video_pipeline_run() is called for every frame, it should
> only be on the first frame of each stream, (or the first frame following
> a resume operation) where the flag pipe->configured == false.
> 
>  - Measuring in the entirety of the vsp-test suite,  I count 121
>    calls here.
>    (trace-cmd report | grep vsp1_video_setup_pipeline | wc -l)
> 
> 
> B) Function duration
> 
> The log of each of the function durations is available at:
>    http://paste.ubuntu.com/23632986/
> 
> Some quick and dirty statistical analysis shows the following duration
> times for this function.
> 
> trace-cmd report \
> 	| grep vsp1_video_setup_pipeline \
> 	| sed 's/.*funcgraph_entry:\W*\([0-9]*\.[0-9]*\) us.*/\1/' \
> 	| st
> 
> ## Numbers in uS ...
> N       min     max     sum     mean    stddev
> 121     4.68    17.76   887.885 7.33789 2.3195
> 
> 
> So we do hit nearly 18 uS in this function, which could be considered a
> lot for interrupt context, but when it is only once at the beginning of
> stream start?
>
> (Incidentally, running the same measurements on the whole of
> vsp1_irq_handler nets the following statistical results:
> 
> trace-cmd report \
> 	| grep vsp1_irq_handler \
> 	| sed 's/.*funcgraph_entry:\W*\([0-9]*\.[0-9]*\) us.*/\1/' \
> 	| st
> 
> N       min     max     sum     mean    stddev
> 12360   1.32    165.481 125934  10.1889 15.0117
> 
> Of course the coverage on the IRQ handler is more broad, and I'm not
> sure how to only measure the IRQ handler calls that result in calling
> vsp1_video_setup_pipeline().
> 

Those interrupt context comparisons are irrelevant now that you've
cleared up the reference was due to the spinlock being held, rather than
the calls being in interrupt context :)


> Anyway, like I said - possibly a moot point anyway - but I wanted to
> actually measure the function times to see what we're up against.
> 
>>>  	if (!pipe->dl)
>>>  		pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
>>>
>>> @@ -793,25 +798,18 @@ static int vsp1_video_start_streaming(struct vb2_queue
>>> *vq, unsigned int count) struct vsp1_video *video = vb2_get_drv_priv(vq);
>>>  	struct vsp1_pipeline *pipe = video->rwpf->pipe;
>>>  	unsigned long flags;
>>> -	int ret;
>>>
>>>  	mutex_lock(&pipe->lock);
>>>  	if (pipe->stream_count == pipe->num_inputs) {
>>> -		ret = vsp1_video_setup_pipeline(pipe);
>>> -		if (ret < 0) {
>>> -			mutex_unlock(&pipe->lock);
>>> -			return ret;
>>> -		}
>>> +		spin_lock_irqsave(&pipe->irqlock, flags);
>>> +		if (vsp1_pipeline_ready(pipe))
>>> +			vsp1_video_pipeline_run(pipe);
>>> +		spin_unlock_irqrestore(&pipe->irqlock, flags);
>>>  	}
>>>
>>>  	pipe->stream_count++;
>>>  	mutex_unlock(&pipe->lock);
>>>
>>> -	spin_lock_irqsave(&pipe->irqlock, flags);
>>> -	if (vsp1_pipeline_ready(pipe))
>>> -		vsp1_video_pipeline_run(pipe);
>>> -	spin_unlock_irqrestore(&pipe->irqlock, flags);
>>> -
>>
>> How about the following ?
>>
>> 	bool start_pipeline = false;
>>
>>  	mutex_lock(&pipe->lock);
>>  	if (pipe->stream_count == pipe->num_inputs) {
>> 		ret = vsp1_video_setup_pipeline(pipe);
>> 		if (ret < 0) {
>> 			mutex_unlock(&pipe->lock);
>> 			return ret;
>> 		}
>>
>> 		start_pipeline = true;
>>  	}
>>
>>  	pipe->stream_count++;
>>  	mutex_unlock(&pipe->lock);
>>
>> 	/*
>> 	 * Don't attempt to start the pipeline if we haven't configured it
>> 	 * explicitly, as otherwise multiple streamon calls could race each
>> 	 * other and one of them try to start the pipeline unconfigured.
>> 	 */
>> 	if (!start_pipeline)
>> 		return 0;
>>
>> 	spin_lock_irqsave(&pipe->irqlock, flags);
>> 	if (vsp1_pipeline_ready(pipe))
>> 		vsp1_video_pipeline_run(pipe);
>> 	spin_unlock_irqrestore(&pipe->irqlock, flags);
> 
> Ok, I'm happy with that, it's effectively v1 of my patchset, before I
> discovered that the suspend resume issue was closely related.
> 
> 
>> This won't fix the suspend/resume issue, but I think splitting that to a 
>> separate patch would be a good idea anyway.
> 

Reverting back to the 'v1' style, and then moving the call to setup the
pipeline, into the pipeline_run() against a configured flag to fix for
suspend/resume cycles, netts the following code :

	mutex_lock(&pipe->lock);
	if (pipe->stream_count == pipe->num_inputs)
		start_pipeline = true;

	pipe->stream_count++;
	mutex_unlock(&pipe->lock);

	/*
	 * vsp1_pipeline_ready() is not sufficient to establish that all streams
	 * are prepared and the pipeline is configured, as multiple streams
	 * can race through streamon with buffers already queued; Therefore we
	 * don't even attempt to start the pipeline until the last stream has
	 * called through here.
	 */
	if (!start_pipeline)
		return 0;

	spin_lock_irqsave(&pipe->irqlock, flags);
	if (vsp1_pipeline_ready(pipe))
		vsp1_video_pipeline_run(pipe);
	spin_unlock_irqrestore(&pipe->irqlock, flags);



> I'll have to look again to consider how this could be done separately.
> 
> 
>> I'm also wondering whether we could have a streamon/suspend race. If the 
>> pipeline is setup and the DL allocated but not committed at suspend time I 
>> think we'll leak the DL at resume time.
> 
> If there is, then I believe I resolved this in :
>  "v4l: vsp1: Use local display lists and remove global pipe->dl"
> 
> but yes, if we end up not being able to use that patch then I'll take a
> look and see if there's anything more needs to be done here.
> 
>>>  	return 0;
>>>  }
>>
> 

-- 
Regards

Kieran Bingham