Message-ID: <289d9c92-383f-3257-de7b-46179724285a@nvidia.com>
Date:   Thu, 30 Apr 2020 14:37: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 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.
>
>> 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