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

From: Sowjanya Komatineni
Date: Sat May 02 2020 - 15:15:58 EST


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