Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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 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;
     }