Date:   Thu, 30 Apr 2020 12:53:26 -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 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

>
>>> My understanding is buffer updates/release should not happen after
>>> frozen state. So we should let frame capture of outstanding buffer to
>>> finish before freezing in capture_finish thread.
>>>
>>> But for capture_start thread we can unconditionally freeze before
>>> dequeuing next buffer for capture.
>>>
>>> With this when both threads are in frozen state and no buffer
>>> updates/captures will happen after frozen state.
>>>
>>> I think its not required to finish streaming of all frames 
>>> completely to
>>> let threads to enter frozen state as streaming can be continuous as 
>>> well.
>> Yes, only freezing in the middle of IO should be avoided.
>>
>> https://lwn.net/Articles/705269/
>>
>>>>> Hi Dmitry,
>>>>>
>>>>> Will update in v12 to not allow freeze in middle of a frame capture.
>>>>>
>>>>> Can you please confirm on above if you agree to allow freeze to
>>>>> happen in b/w frame captures?
>>>>>
>>>>> Also as most feedback has been received from you by now, appreciate
>>>>> if you can provide all in this v11 if you have anything else so we
>>>>> will not have any new changes after v12.
>> I'll take another look tomorrow / during weekend and let you know.

Powered by blists - more mailing lists