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

[Sowjanya Komatineni]

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