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 10:04:01 -0700 |
| |
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; >>>> } >>>> >
| |