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

Sowjanya Komatineni
2020-05-02T19:15:35Z

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