Date:   Sat, 2 May 2020 10:04:01 -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 5/2/20 9:55 AM, Sowjanya Komatineni wrote:
>
> On 5/2/20 9:14 AM, Sowjanya Komatineni wrote:
>>
>> On 5/2/20 9:03 AM, Sowjanya Komatineni wrote:
>>>
>>> On 5/2/20 8:38 AM, Sowjanya Komatineni wrote:
>>>>
>>>> On 5/2/20 8:16 AM, Dmitry Osipenko wrote:
>>>>> 02.05.2020 06:55, Sowjanya Komatineni пишет:
>>>>>> On 5/1/20 8:39 PM, Sowjanya Komatineni wrote:
>>>>>>>
>>>>>>> On 5/1/20 2:05 PM, Sowjanya Komatineni wrote:
>>>>>>>>
>>>>>>>> On 5/1/20 1:58 PM, Sowjanya Komatineni wrote:
>>>>>>>>>
>>>>>>>>> On 5/1/20 1:44 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>
>>>>>>>>>> On 5/1/20 11:03 AM, Sowjanya Komatineni wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 4/30/20 4:33 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 4/30/20 4:14 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>
>>>>>>>>>>>>> try_to_freeze() returns thread frozen state and looks like we
>>>>>>>>>>>>> can use this in kthread finish to allow finish thread to 
>>>>>>>>>>>>> freeze
>>>>>>>>>>>>> only when kthread_start is already frozen and no buffers in
>>>>>>>>>>>>> progress/initiated for capture.
>>>>>>>>>>>>>
>>>>>>>>>>>> chan->capture_frozen holds frozen state returned from
>>>>>>>>>>>> try_to_freeze() in start kthread
>>>>>>>>>>>>
>>>>>>>>>>>> chan->capture_reqs increments after every single shot issued.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> static int chan_capture_kthread_finish(void *data)
>>>>>>>>>>>>
>>>>>>>>>>>> {
>>>>>>>>>>>>      struct tegra_vi_channel *chan = data;
>>>>>>>>>>>>      struct tegra_channel_buffer *buf;
>>>>>>>>>>>>      int caps_inflight;
>>>>>>>>>>>>
>>>>>>>>>>>>      set_freezable();
>>>>>>>>>>>>
>>>>>>>>>>>>      while (1) {
>>>>>>>>>>>> wait_event_interruptible(chan->done_wait,
>>>>>>>>>>>>  !list_empty(&chan->done) ||
>>>>>>>>>>>>  kthread_should_stop());
>>>>>>>>>>>>
>>>>>>>>>>>>          /* dequeue buffers and finish capture */
>>>>>>>>>>>>          buf = dequeue_buf_done(chan);
>>>>>>>>>>>>          while (buf) {
>>>>>>>>>>>> tegra_channel_capture_done(chan, buf);
>>>>>>>>>>>>              buf = dequeue_buf_done(chan);
>>>>>>>>>>>>          }
>>>>>>>>>>>>
>>>>>>>>>>>>          if (kthread_should_stop())
>>>>>>>>>>>>              break;
>>>>>>>>>>>>
>>>>>>>>>>>>          caps_inflight = chan->capture_reqs - chan->sequence;
>>>>>>>>>>>>          if (chan->capture_frozen && !caps_inflight)
>>>>>>>>>>>>              try_to_freeze();
>>>>>>>>>>>>      }
>>>>>>>>>>>>
>>>>>>>>>>>>      return 0;
>>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> Freezing happens prior to suspend() during suspend entry and 
>>>>>>>>>>> when
>>>>>>>>>>> we implement suspend/resume during suspend we stop streaming 
>>>>>>>>>>> where
>>>>>>>>>>> we stop threads anyway.
>>>>>>>>>>>
>>>>>>>>>>> So, was thinking why we need these threads freezable here?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> Did some testing and below are latest observation and fix I 
>>>>>>>>>> tested.
>>>>>>>>>>
>>>>>>>>>> wait_event_interruptible() uses schedule() which blocks the 
>>>>>>>>>> freezer.
>>>>>>>>>> When I do suspend while keeping streaming active in 
>>>>>>>>>> background, I
>>>>>>>>>> see freezing of these threads fail and call trace shows 
>>>>>>>>>> __schedule
>>>>>>>>>> -> __switch_to from these kthreads.
>>>>>>>>>>
>>>>>>>>>> wait_event_freezable() uses freezable_schedule() which should 
>>>>>>>>>> not
>>>>>>>>>> block the freezer but we can't use this here as we need 
>>>>>>>>>> conditional
>>>>>>>>>> try_to_freeze().
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> So, doing below sequence works where we set PF_FREEZER_SKIP flag
>>>>>>>>>> thru freezer_not_count() before wait_event which calls 
>>>>>>>>>> schedule()
>>>>>>>>>> and remove PF_FREEZER_SKIP after schedule allows 
>>>>>>>>>> try_to_freeze to
>>>>>>>>>> work and also conditional try_to_freeze below prevents freezing
>>>>>>>>>> thread in middle of capture.
>>>>>>>>>>
>>>>>>>>>> while (1) {
>>>>>>>>>>      freezer_not_count()
>>>>>>>>>>      wait_event_interruptible()
>>>>>>>>>>      freezer_count()
>>>>>>>>>>      ...
>>>>>>>>>>      ...
>>>>>>>>>>      if (chan->capture_frozen && !caps_inflight)
>>>>>>>>>>          try_to_freeze()
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> Please comment if you agree with above sequence. Will include 
>>>>>>>>>> this
>>>>>>>>>> in v12.
>>>>>>>>>>
>>>>>>>> sorry, freezer_count() does try_to_freeze after clearing skip 
>>>>>>>> flag.
>>>>>>>> So, dont think we can use this as we need conditional 
>>>>>>>> try_to_freeze.
>>>>>>>> Please ignore above sequence.
>>>>>>>>> Or probably we can take closer look on this later when we add
>>>>>>>>> suspend/resume support as it need more testing as well.
>>>>>>>>>
>>>>>>>>> As this is initial series which has TPG only I think we shouldn't
>>>>>>>>> get blocked on this now. Series-2 and 3 will be for sensor 
>>>>>>>>> support
>>>>>>>>> and on next series when we add suspend/resume will look into 
>>>>>>>>> this.
>>>>>>>>>
>>>>>>>>>
>>>>>>> When freeze activity starts and in case if finish thread freezes 
>>>>>>> prior
>>>>>>> to start thread issuing capture, its the VI hardware writes data to
>>>>>>> the allocated buffer address.
>>>>>>>
>>>>>>> finish thread just checks for the event from the hardware and we 
>>>>>>> don't
>>>>>>> handle/process directly on memory in this driver.
>>>>>>>
>>>>>>> So even we freeze done thread when single shot is issued frame 
>>>>>>> buffer
>>>>>>> gets updated.
>>>>>>>
>>>>>>> In case if capture thread is frozen there will not buffers 
>>>>>>> queued to
>>>>>>> process by finish thread. So, this will not be an issue.
>>>>>>>
>>>>>>> So, probably we don't need to do conditional try_to_freeze and 
>>>>>>> what we
>>>>>>> have should work good in this corner case.
>>>>>>>
>>>>>> I still need to change wait_event_interruptible() to
>>>>>> wait_event_freezable() but no need to synchronize finish thread 
>>>>>> freeze
>>>>>> with start thread as even on issuing capture start its vi 
>>>>>> hardware that
>>>>>> does frame buffer update and finish thread just checks for mw_ack 
>>>>>> event
>>>>>> and returns buffer to application.
>>>>> The problem we are primarily trying to avoid is to have suspending 
>>>>> being
>>>>> done in the middle of IO.
>>>>>
>>>>> IIUC, even if system will be suspended in the middle of VI IO, it 
>>>>> won't
>>>>> be fatal. In worst case the buffer capture should fail on resume from
>>>>> suspend. Could you please try to simulate this potential issue and 
>>>>> see
>>>>> what result will be on suspending in the middle of VI IO?
>>>>>
>>>>> We don't want to suspend system / stop streaming in the middle of 
>>>>> IO, so
>>>>> this problem of a proper threads tear-down still exists. It should
>>>>> become easier to resolve the problem in a case of a proper suspending
>>>>> callback because the "start" thread could be turned down at any 
>>>>> time, so
>>>>> it should be easier to maintain a proper tear-down order when threads
>>>>> are fully controlled by the driver, i.e. the "start" thread goes down
>>>>> first and the "finish" is second, blocking until the capture is 
>>>>> completed.
>>>
>>> I don't see issue of tear-down threads in case of suspend as we do 
>>> stop streaming where thread stop happens on both threads and are 
>>> stopped only after processing all outstanding buffers.
>>>
>>> Regarding freezing activity during suspend, If done thread freezes 
>>> prior to processing buffers for finish, vi hardware is still active 
>>> by this time which will update the frame buffer for initiated 
>>> capture. Driver is not directly involved in this frame buffer update.
>>>
>>> Finish thread only checks for completion to return buffers back to 
>>> the application when done.
>>
>> when done thread freeze happens after start thread initiated capture, 
>> vi hardware continues to update frame buffer for ongoing capture till 
>> it hits driver suspend callback. Yes worst case this frame data may 
>> not be valid data if invoking of this driver suspend happens 
>> immediate after this thread freeze during system suspend.
>>
>> But driver will still hold buffers to return which will be returned 
>> back on resume when threads are out from frozen state.
>
>
> Also stop stream ioctl request happens during suspend where both 
> threads will be stopped properly. done thread stop happens only after 
> finishing all outstanding buffers.
>
> Stop stream request happens from streaming applications so even 
> without driver suspend/resume implementation currently, streaming will 
> be stopped prior to system  suspend where both threads will be stopped 
> properly (after finishing out standing buffers) and will be resumed by 
> application on system resume
>
> Also tested suspending while streaming with this unconditional freeze, 
> I don't see any issue as application stops stream where v4l_streamoff 
> gets executed during suspend and on resume streaming starts where 
> v4l_streamon happens.
>
> So, I don't see any issue with existing implementation of 
> unconditional freeze.

To be more clear,

suspend while streaming use case...

- start thread initiating capture
- done thread frozen (with outstanding buffers in process but vi 
hardware still continues to update frame buffer)
- start thread frozen

application holding video device will issue stream off ioctl
- stop_streaming does kthread_stop

- threads wake up and start thread breaks and finish thread breaks after 
checking outstanding buffers and returning to application

- vi/csi power/clocks off


on resume, application starts streaming again where fresh start stream 
happens.

>
>>
>>>
>>>
>>>>> I think yours suggestion about dropping the freezing from the threads
>>>>> for now and returning back to it later on (once a proper 
>>>>> suspend/resume
>>>>> support will be added) sounds reasonable.
>>>>>
>>>>> But if you'd want to keep the freezing, then the easy solution 
>>>>> could be
>>>>> like that:
>>>>>
>>>>>    1. "start" thread could freeze at any time
>>>>>    2. "finish" thread could freeze only when the "start" thread is 
>>>>> frozen
>>>>> and capture isn't in-progress. Use frozen(kthread_start_capture) to
>>>>> check the freezing state.
>>>>>
>>>>> https://elixir.bootlin.com/linux/v5.7-rc3/source/include/linux/freezer.h#L25 
>>>>>
>>>>
>>>> That's exactly what I tried, below is the snippet.
>>>>
>>>> But as mentioned I am seeing freezing fail when I 
>>>> wait_event_interruptible() in either of the threads.
>>>>
>>>>    60.368709] Call trace:
>>>> [   60.371216] __switch_to+0xec/0x140
>>>> [   60.374768] __schedule+0x32c/0x668
>>>> [   60.378315] schedule+0x78/0x118
>>>> [   60.381606]  chan_capture_kthread_finish+0x244/0x2a0 [tegra_video]
>>>> [   60.387865] kthread+0x124/0x150
>>>> [   60.391150] ret_from_fork+0x10/0x1c
>>>>
>>>> wait_event_interruptible() API uses schedule() which blocks freezer 
>>>> while wait_event_freezable APIs uses freezable_schedule() which 
>>>> allows to skip freezer during schedule and then clears skip and 
>>>> calls try_to_freeze()
>>>>
>>>> But we can't use wait_event_freezable() here as we need conditional 
>>>> freeze.
>>>>
>>>>
>>>>     while (1) {
>>>>         caps_inflight = chan->capture_reqs - chan->sequence;
>>>>         if (frozen(chan->kthread_start_capture) && !caps_inflight)
>>>>             wait_event_freezable(chan->done_wait,
>>>>                          !list_empty(&chan->done) ||
>>>>                          kthread_should_stop());
>>>>         else
>>>>             wait_event_interruptible(chan->done_wait,
>>>>                          !list_empty(&chan->done) ||
>>>>                          kthread_should_stop());
>>>>
>>>>         /* dequeue buffers and finish capture */
>>>>
>>>>         ...
>>>>
>>>>         ...
>>>>
>>>>
>>>>         if (kthread_should_stop())
>>>>             break;
>>>>     }
>>>>
>

Powered by blists - more mailing lists