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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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