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

[Sowjanya Komatineni <skomatineni@nvidia.com>]

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
>