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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 4/30/20 3:19 PM, Sowjanya Komatineni wrote:
>
> On 4/30/20 3:16 PM, Sowjanya Komatineni wrote:
>>
>> On 4/30/20 2:53 PM, Sowjanya Komatineni wrote:
>>>
>>> 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
>>>
>> Sorry correction. When system tries to freeze tasks looks like it 
>> will sending signal to thread and wake up happens when signal is sent 
>> to thread and freezable thread should invoke try_to_free when its 
>> safe to free
>
> freeze_task() sends fake signal
>
> https://elixir.bootlin.com/linux/v5.7-rc2/source/kernel/freezer.c#L115
>
>>
>>>
>>>>>
>>>>>> 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
> will have caps inflight check in v12 to allow freeze finish thread 
> only when no captures are in progress


try_to_freeze() returns thread frozen state and looks like we can use 
this in kthread finish to allow finish thread to freeze only when 
kthread_start is already frozen and no buffers in progress/initiated for 
capture.