Re: [RFC PATCH v2 11/18] media: tegra-video: Add support for external sensor capture

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux