lkml.org 
[lkml]   [2020]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver
    From
    Date

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

    \
     
     \ /
      Last update: 2020-05-02 21:16    [W:4.024 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site