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

[Sowjanya Komatineni]

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..

> Regards,
>
> Sowjanya