Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 4/30/20 2:37 PM, Sowjanya Komatineni wrote:
>
> On 4/30/20 2:26 PM, Sowjanya Komatineni wrote:
>>
>> On 4/30/20 2:17 PM, Dmitry Osipenko wrote:
>>> 30.04.2020 23:02, Sowjanya Komatineni пишет:
>>>> On 4/30/20 12:53 PM, Sowjanya Komatineni wrote:
>>>>> On 4/30/20 12:46 PM, Sowjanya Komatineni wrote:
>>>>>> On 4/30/20 12:33 PM, Dmitry Osipenko wrote:
>>>>>>> 30.04.2020 22:09, Sowjanya Komatineni пишет:
>>>>>>>> On 4/30/20 11:18 AM, Sowjanya Komatineni wrote:
>>>>>>>>> On 4/30/20 10:06 AM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 4/30/20 9:29 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 4/30/20 9:04 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 4/30/20 7:13 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 30.04.2020 17:02, Dmitry Osipenko пишет:
>>>>>>>>>>>>>> 30.04.2020 16:56, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>> 30.04.2020 01:00, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> +static int chan_capture_kthread_finish(void *data)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> +    struct tegra_vi_channel *chan = data;
>>>>>>>>>>>>>>>> +    struct tegra_channel_buffer *buf;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +    set_freezable();
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +    while (1) {
>>>>>>>>>>>>>>>> +        try_to_freeze();
>>>>>>>>>>>>>>> I guess it won't be great to freeze in the middle of a 
>>>>>>>>>>>>>>> capture
>>>>>>>>>>>>>>> process, so:
>>>>>>>>>>>>>>>           if (list_empty(&chan->done))
>>>>>>>>>>>>>>>               try_to_freeze();
>>>>>>>>>>>>>> And here should be some locking protection in order not race
>>>>>>>>>>>>>> with
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> chan_capture_kthread_start because kthread_finish could 
>>>>>>>>>>>>>> freeze
>>>>>>>>>>>>>> before
>>>>>>>>>>>>>> kthread_start.
>>>>>>>>>>>>> Or maybe both start / finish threads should simply be 
>>>>>>>>>>>>> allowed to
>>>>>>>>>>>>> freeze
>>>>>>>>>>>>> only when both capture and done lists are empty.
>>>>>>>>>>>>>
>>>>>>>>>>>>> if (list_empty(&chan->capture) &&
>>>>>>>>>>>>>        list_empty(&chan->done))
>>>>>>>>>>>>>       try_to_freeze();
>>>>>>>>>>>> good to freeze when not in middle of the frame capture but why
>>>>>>>>>>>> should we not allow freeze in between captures?
>>>>>>>>>>>>
>>>>>>>>>>>> Other drivers do allow freeze in between frame captures.
>>>>>>>>>>>>
>>>>>>>>>>>> I guess we can freeze before dequeue for capture and in finish
>>>>>>>>>>>> thread we can freeze after capture done. This also don't 
>>>>>>>>>>>> need to
>>>>>>>>>>>> check for list_empty with freeze to allow between frame 
>>>>>>>>>>>> captures.
>>>>>>>>>>>>
>>>>>>>>>>> Also if we add check for both lists empty, freeze is not 
>>>>>>>>>>> allowed as
>>>>>>>>>>> long as streaming is going on and in case of continuous 
>>>>>>>>>>> streaming
>>>>>>>>>>> freeze will never happen.
>>>>>>>>> To allow freeze b/w frames (but not in middle of a frame),
>>>>>>>>>
>>>>>>>>> for capture_start thread, probably we can do unconditional
>>>>>>>>> try_to_freeze()
>>>>>>> Is it possible to use wait_event_freezable()?
>>>>>>>
>>>>>>> https://www.kernel.org/doc/Documentation/power/freezing-of-tasks.txt 
>>>>>>>
>>>>>>>
>>>>>>> Will the wait_event_interruptible() be woken up when system 
>>>>>>> freezes?
>>>>>> Based on wait_event_freezable implementation, looks like it similar
>>>>>> to wait_event_interruptible + try_to_free() as it does
>>>>>> freezable_schedule unlike schedule with wait_event_interruptible.
>>>>>>
>>>>>> So using this for capture_start may be ok to allow freeze before
>>>>>> start of frame. But can't use for capture_finish as this is same as
>>>>>> wait_event_interruptible followed by unconditional try_to_freeze.
>>>>>>
>>>>>>>>> for capture_finish thread, at end of capture done we can do
>>>>>>>>> try_to_freeze() only when done list is empty
>>>>>>> This doesn't prevent situation where the done-list is empty and the
>>>>>>> "finish" thread freezes, in the same time the "start" thread 
>>>>>>> issues new
>>>>>>> capture and then freezes too.
>>>>>>>
>>>>>>> 1. "start" thread issues capture
>>>>>>>
>>>>>>> 2. "finish" thread wakes and waits for the capture to complete
>>>>>>>
>>>>>>> 3. "start" thread begins another capture, waits for FRAME_START
>>>>>>>
>>>>>>> 4. system freezing activates
>>>>>>>
>>>>>>> 5. "finish" thread completes the capture and freezes because 
>>>>>>> done-list
>>>>>>> is empty
>>>>>>>
>>>>>>> 6. "start" thread gets FRAME_START, issues another capture and 
>>>>>>> freezes
>>>>>> This will not happen as we allow double buffering done list will not
>>>>>> be empty till stream stop happens
>>>>>>
>>>>>> There will always be 1 outstanding frame in done list
>>>>> Correction, there will always be 1 outstanding buffer except 
>>>>> beginning
>>>>> during beginning of stream.
>>>>>
>>>>> Except during beginning frames, done list will not be empty for all
>>>>> subsequent streaming process
>>>> Also to be clear, hardware sees next frame start event prior to 
>>>> previous
>>>> frame mw_ack event as mw_ack event happens after frame end. So once
>>>> initial buffer got queued to done list to finish processes, while
>>>> waiting for mw_ack next frame start happens and pushes next buffer to
>>>> done list.
>>> What about this variant:
>>>
>>> 1. "start" thread wakes up to start capture
>>>
>>> 2. system freezing activates
>>>
>>> 3. "finish" thread wakes up and freezes
>>
>> finish thread will wake up only when done list is not 
>> empty/kthread_stop/wake even from capture start thread.
>>
>> Also when I said will allow try_to_free when done list is empty I 
>> meant to have this at end of capture_done() in finish thread
>>
>>>
>>> 4. "start" thread issues capture and freezes
>>>
>>> And again, I assume that system's freezing should wake
>>> wait_event_interruptible(), otherwise it won't be possible to freeze
>>> idling threads at all and freezing should fail (IIUC).
>>
>> Based on kernel doc on freezing, looks like when we mark thread as 
>> freezable, freeze state happens when we explicitly call try_to_freeze.
>>
>> I don't think its other way where freeze causes 
>> wait_event_interruptible to wake up.

Based on my understanding when we mark thread as freezable,

with wait_event_freezable() - after wait event, it invokes 
try_to_freeze(). So frozen state enters unconditionally with this.

with wait_event_interruptible - we do try_to_freeze when its safe to 
enter frozen state.

https://www.kernel.org/doc/Documentation/power/freezing-of-tasks.txt


>>
>>> And in this case synchronization between start/finish threads should be
>>> needed in regards to freezing.
>>
>> Was thinking to have counter to track outstanding frame w.r.t single 
>> shot issue b/w start and finish and allow to freeze only when no 
>> outstanding frames in process.
>>
>> This will make sure freeze will not happen when any buffers are in 
>> progress
>>
>>> Note that this could be a wrong assumption, I'm not closely familiar
>>> with how freezer works.
>
> kthread_start can unconditionally allow try_to_freeze before start of 
> frame capture
>
> We can compute captures inflight w.r.t single shot issued during 
> capture start and finished frames by kthread_finish and allow 
> kthread_finish to freeze only when captures inflight is 0.
>
> This allows freeze to happen b/w frames but not in middle of frame
>
>