Message-ID: <bf3f654e-b8f8-d560-fc5e-03d73cb7eab0@nvidia.com>
Date:   Thu, 30 Apr 2020 15:19:00 -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 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
>>>
>>>

Powered by blists - more mailing lists