Date:   Thu, 30 Apr 2020 15:16:12 -0700
From:   Sowjanya Komatineni <skomatineni@...dia.com>
To:     Dmitry Osipenko <digetx@...il.com>, <thierry.reding@...il.com>,
        <jonathanh@...dia.com>, <frankc@...dia.com>, <hverkuil@...all.nl>,
        <sakari.ailus@....fi>, <helen.koike@...labora.com>
CC:     <sboyd@...nel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>,
        <linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver


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

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

Powered by blists - more mailing lists