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

Sowjanya Komatineni
2020-05-02T17:04:43Z

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