Message-ID: <4022a7a7-2ea0-dc1d-4dbc-29985ca3f315@nvidia.com>
Date:   Sat, 2 May 2020 12:14:47 -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 10:04 AM, Sowjanya Komatineni wrote:
>
> 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;
>>>>>     }
>>>>>
>>
Below works if I skip freezer during wait_event_interruptible as we 
can't use wait_event_freezable when we need conditional try_to_freeze()

But I am not sure if this is allowed to do this explicit PF_FREEZER_SKIP 
clear.

Also, as explained above I don't even think we need conditional 
try_to_freeze() as v4l2 ioctl stream off gets called during closing of 
device node when suspend happens and before stopping threads outstanding 
buffers are returned properly. So, I don't see any issue with 
unconditional try_to_freeze()

     while (1) {

         /* allow to freeze only when no captures in progress */
         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 {
             /*
              * Skip freeze during wait_event_interruptible as
              * schedule() blocks freezer.
              */
             if (freezing(current))
                 freezer_do_not_count();

             wait_event_interruptible(chan->done_wait,
                          !list_empty(&chan->done) ||
                          kthread_should_stop());
             if (freezing(current))
                 current->flags &= ~PF_FREEZER_SKIP;
         }

         /* dequeue buffers and finish capture */

         ...

         ...


         if (kthread_should_stop())
             break;
     }

Powered by blists - more mailing lists