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

[Dmitry Osipenko <digetx@gmail.com>]

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

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).

And in this case synchronization between start/finish threads should be
needed in regards to freezing.

Note that this could be a wrong assumption, I'm not closely familiar
with how freezer works.