Messages in this thread | | | Subject | Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver | From | Sowjanya Komatineni <> | Date | Sat, 2 May 2020 08:38:37 -0700 |
| |
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; }
| |