On 7/7/20 1:41 PM, Sowjanya Komatineni wrote:
On 7/7/20 1:29 PM, Sowjanya Komatineni wrote:
On 7/7/20 12:35 PM, Hans Verkuil wrote:
On 07/07/2020 21:25, Sowjanya Komatineni wrote:
On 7/7/20 12:01 PM, Sowjanya Komatineni wrote:
On 7/6/20 2:10 AM, Hans Verkuil wrote:
+static void tegra_vi_graph_cleanup(struct tegra_vi *vi)
+{
+ struct tegra_vi_channel *chan;
+
+ list_for_each_entry(chan, &vi->vi_chans, list) {
+ video_unregister_device(&chan->video);
+ mutex_lock(&chan->video_lock);
+ vb2_queue_release(&chan->queue);
No need for this since this is done in vb2_fop_release().
In fact, vb2_queue_release should never be called by drivers.
Just using
vb2_fop_release or __vb2_fop_release is sufficient.
The confusion is due to the fact that the name suggests that
vb2_queue_release
has to be balanced with vb2_queue_init, but that's not the case.
Perhaps
vb2_queue_stop or something like that might be a better name.
I'll have to
think about this since I see that a lot of drivers do this wrong.
+ mutex_unlock(&chan->video_lock);
+ v4l2_async_notifier_unregister(&chan->notifier);
+ v4l2_async_notifier_cleanup(&chan->notifier);
+ }
+}
+
vb2_queue_release() here is called to stop streaming a head before
media links are removed in case of when driver unbind happens while
userspace application holds video device with active streaming in
progress.
Without vb2_queue_release() here streaming will be active during
the driver unbind and by the time vb2_queue_release() happens from
vb2_fop_release(), async notifiers gets unregistered and media
links will be removed which causes channel stop stream to crash as
we can't
retrieve sensor subdev thru media entity pads to execute s_stream
on subdev.
I think we don't need async notifier unbind. Currently media links
are removed during unbind so during notifier unregister all subdevs
gets
unbind and links removed.
media_device_unregister during video device release callback takes
care of media entity unregister and removing links.
So, will try by removing notifier unbind along with removing
vb2_queue_release during cleanup.
I actually wonder if vb2_queue_release shouldn't be called from
video_unregister_device.
I'll look into this tomorrow.
Regards,
Hans
Thanks Hans.
Tried without notifier unbind to remove media links and I still see
crash due to below diff reason now.
With userspace app holding video device node with active streaming in
progress when I do driver unbind, v4l2_device release callback
tegra_v4l2_dev_release() happens prior to vb2_fops_release() ->
vb2_queue_release().
All channels resources and channel memory is freed during v4l2_device
release callback.
Letting vb2_queue_release() to happen thru vb2_fops_release() causes
crash as stop streaming tries to retrieve subdev thru channel media
pads and channel memory is freed by that time.
So, doing vb2_queue_release() during driver unbind -> tegra_vi_exit()
-> tegra_vi_graph_cleanup(), stops subdev stream properly and then on
v4l2_device release channel memory gets freed and this works which is
the existing implementation in the patch.
I remember adding vb2_queue_release() during graph cleanup for TPG as
well for the same reason to allow driver unbind while holding video
device from user space so media pad can be accessible to stop stream
before channel cleanup.
v4l2_dev release() should definitely happen in the last after
vb2_fops_release(). Will add more debugs and confirm on what I
observed as something happened with timestamps on log on my side so I
doubt my above observation after removing notifier unbind to remove
media links.
Will check and get back..
Hi Hans,
Please ignore above observation on channel cleanup during v4l2_dev
release and vb2_fops_release. I misunderstood from timestamps.
Looking into vdev->dev release callback v4l2_device_release(), it
unregisters media entity and links gets removed.
So, by the time vb2_queue_release() happen thru vb2_fops_release(),
links are removed so subdev can't be retrieve thru media entity pads
unless video driver maintains subdev pointers to use them instead of
retrieving from media entity pads.
To fix this, instead of maintaining subdev pointers in Tegra video
driver channel structure, probably we can leave vb2_queue_release() call
to happen during driver unbind graph cleanup which does stream stop
where Tegra channel stop stream retrieves subdev thru media pad entities
when media links are still present.
Thanks
Sowjanya
Regards,
Sowjanya
