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.
Regards,
Sowjanya
