Messages in this thread | | | Subject | Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver | From | Sowjanya Komatineni <> | Date | Thu, 30 Apr 2020 12:53:26 -0700 |
| |
On 4/30/20 12:46 PM, Sowjanya Komatineni wrote: > > On 4/30/20 12:33 PM, Dmitry Osipenko wrote: >> 30.04.2020 22:09, Sowjanya Komatineni пишет: >>> On 4/30/20 11:18 AM, Sowjanya Komatineni wrote: >>>> On 4/30/20 10:06 AM, Sowjanya Komatineni wrote: >>>>> On 4/30/20 9:29 AM, Sowjanya Komatineni wrote: >>>>>> On 4/30/20 9:04 AM, Sowjanya Komatineni wrote: >>>>>>> On 4/30/20 7:13 AM, Dmitry Osipenko wrote: >>>>>>>> 30.04.2020 17:02, Dmitry Osipenko пишет: >>>>>>>>> 30.04.2020 16:56, Dmitry Osipenko пишет: >>>>>>>>>> 30.04.2020 01:00, Sowjanya Komatineni пишет: >>>>>>>>>>> +static int chan_capture_kthread_finish(void *data) >>>>>>>>>>> +{ >>>>>>>>>>> + struct tegra_vi_channel *chan = data; >>>>>>>>>>> + struct tegra_channel_buffer *buf; >>>>>>>>>>> + >>>>>>>>>>> + set_freezable(); >>>>>>>>>>> + >>>>>>>>>>> + while (1) { >>>>>>>>>>> + try_to_freeze(); >>>>>>>>>> I guess it won't be great to freeze in the middle of a capture >>>>>>>>>> process, so: >>>>>>>>>> if (list_empty(&chan->done)) >>>>>>>>>> try_to_freeze(); >>>>>>>>> And here should be some locking protection in order not race with >>>>>>>>> the >>>>>>>>> chan_capture_kthread_start because kthread_finish could freeze >>>>>>>>> before >>>>>>>>> kthread_start. >>>>>>>> Or maybe both start / finish threads should simply be allowed to >>>>>>>> freeze >>>>>>>> only when both capture and done lists are empty. >>>>>>>> >>>>>>>> if (list_empty(&chan->capture) && >>>>>>>> list_empty(&chan->done)) >>>>>>>> try_to_freeze(); >>>>>>> good to freeze when not in middle of the frame capture but why >>>>>>> should we not allow freeze in between captures? >>>>>>> >>>>>>> Other drivers do allow freeze in between frame captures. >>>>>>> >>>>>>> I guess we can freeze before dequeue for capture and in finish >>>>>>> thread we can freeze after capture done. This also don't need to >>>>>>> check for list_empty with freeze to allow between frame captures. >>>>>>> >>>>>> Also if we add check for both lists empty, freeze is not allowed as >>>>>> long as streaming is going on and in case of continuous streaming >>>>>> freeze will never happen. >>>> To allow freeze b/w frames (but not in middle of a frame), >>>> >>>> for capture_start thread, probably we can do unconditional >>>> try_to_freeze() >> Is it possible to use wait_event_freezable()? >> >> https://www.kernel.org/doc/Documentation/power/freezing-of-tasks.txt >> >> Will the wait_event_interruptible() be woken up when system freezes? > > Based on wait_event_freezable implementation, looks like it similar to > wait_event_interruptible + try_to_free() as it does freezable_schedule > unlike schedule with wait_event_interruptible. > > So using this for capture_start may be ok to allow freeze before start > of frame. But can't use for capture_finish as this is same as > wait_event_interruptible followed by unconditional try_to_freeze. > >> >>>> for capture_finish thread, at end of capture done we can do >>>> try_to_freeze() only when done list is empty >> This doesn't prevent situation where the done-list is empty and the >> "finish" thread freezes, in the same time the "start" thread issues new >> capture and then freezes too. >> >> 1. "start" thread issues capture >> >> 2. "finish" thread wakes and waits for the capture to complete >> >> 3. "start" thread begins another capture, waits for FRAME_START >> >> 4. system freezing activates >> >> 5. "finish" thread completes the capture and freezes because done-list >> is empty >> >> 6. "start" thread gets FRAME_START, issues another capture and freezes > > This will not happen as we allow double buffering done list will not > be empty till stream stop happens > > There will always be 1 outstanding frame in done list
Correction, there will always be 1 outstanding buffer except beginning during beginning of stream.
Except during beginning frames, done list will not be empty for all subsequent streaming process
> >>> My understanding is buffer updates/release should not happen after >>> frozen state. So we should let frame capture of outstanding buffer to >>> finish before freezing in capture_finish thread. >>> >>> But for capture_start thread we can unconditionally freeze before >>> dequeuing next buffer for capture. >>> >>> With this when both threads are in frozen state and no buffer >>> updates/captures will happen after frozen state. >>> >>> I think its not required to finish streaming of all frames >>> completely to >>> let threads to enter frozen state as streaming can be continuous as >>> well. >> Yes, only freezing in the middle of IO should be avoided. >> >> https://lwn.net/Articles/705269/ >> >>>>> Hi Dmitry, >>>>> >>>>> Will update in v12 to not allow freeze in middle of a frame capture. >>>>> >>>>> Can you please confirm on above if you agree to allow freeze to >>>>> happen in b/w frame captures? >>>>> >>>>> Also as most feedback has been received from you by now, appreciate >>>>> if you can provide all in this v11 if you have anything else so we >>>>> will not have any new changes after v12. >> I'll take another look tomorrow / during weekend and let you know.
| |