Re: [RFC PATCH v5 0/9] Add Tegra driver for video capture

[Sowjanya Komatineni <skomatineni@nvidia.com>]

On 4/3/20 12:19 AM, Hans Verkuil wrote:
> External email: Use caution opening links or attachments
>
>
> On 4/3/20 7:45 AM, Sowjanya Komatineni wrote:
>> On 3/30/20 9:16 AM, Sowjanya Komatineni wrote:
>>> On 3/30/20 4:02 AM, Hans Verkuil wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 3/30/20 12:04 PM, Hans Verkuil wrote:
>>>>> Hi Sowjanya,
>>>>>
>>>>> On 3/23/20 6:52 PM, Sowjanya Komatineni wrote:
>>>>>> This series adds Tegra210 VI and CSI driver for built-in test pattern
>>>>>> generator (TPG) capture.
>>>>>>
>>>>>> Tegra210 supports max 6 channels on VI and 6 ports on CSI where each
>>>>>> CSI port is one-to-one mapped to VI channel for video capture.
>>>>>>
>>>>>> This series has TPG support only where it creates hard media links
>>>>>> between CSI subdevice and VI video device without device graphs.
>>>>>>
>>>>>> v4l2-compliance results are available below the patch diff.
>>>>>>
>>>>>> [v5]:        Includes,
>>>>>>        - v4 feedback
>>>>>>        - fix for venc powergate mc reset order.
>>>>>>        - fix to have unbind and bind work during v4l2-ctl sleep and
>>>>>> streaming.
>>>>> Unfortunately, I still crash on this.
>>>>>
>>>>> I do the following:
>>>>>
>>>>> Run: v4l2-ctl --stream-mmap
>>>>>
>>>>> Then, from another shell as root:
>>>>>
>>>>> cd /sys/devices/platform/50000000.host1x/tegra-video/driver
>>>>> echo -n tegra-video > unbind
>>>>>
>>>>> I get this crash:
>>>>>
>>>>> [  315.691971] Unable to handle kernel NULL pointer dereference at
>>>>> virtual address 00000000000000b0
>>>>> [  315.700749] Mem abort info:
>>>>> [  315.703536]   ESR = 0x96000004
>>>>> [  315.706587]   EC = 0x25: DABT (current EL), IL = 32 bits
>>>>> [  315.711886]   SET = 0, FnV = 0
>>>>> [  315.714933]   EA = 0, S1PTW = 0
>>>>> [  315.718064] Data abort info:
>>>>> [  315.720936]   ISV = 0, ISS = 0x00000004
>>>>> [  315.724763]   CM = 0, WnR = 0
>>>>> [  315.727726] user pgtable: 4k pages, 48-bit VAs,
>>>>> pgdp=0000000178ee8000
>>>>> [  315.734152] [00000000000000b0] pgd=0000000000000000
>>>>> [  315.739024] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>>>>> [  315.744584] Modules linked in: r8152 nouveau lp855x_bl tegra_drm ttm
>>>>> [  315.750942] CPU: 3 PID: 2206 Comm: bash Tainted: G W
>>>>> 5.6.0-rc1-arm64 #118
>>>>> [  315.759017] Hardware name: NVIDIA Jetson TX1 Developer Kit (DT)
>>>>> [  315.764927] pstate: 20000085 (nzCv daIf -PAN -UAO)
>>>>> [  315.769718] pc : _raw_write_lock_irqsave+0xb0/0x2b8
>>>>> [  315.774590] lr : ida_free+0x48/0x158
>>>>> [  315.778155] sp : ffff800011d8bba0
>>>>> [  315.781462] x29: ffff800011d8bba0 x28: ffff0000f4095400
>>>>> [  315.786766] x27: 0000000000000000 x26: 0000000000000000
>>>>> [  315.792070] x25: 0000000000000000 x24: 0000000000000000
>>>>> [  315.797372] x23: ffff0000f21ad400 x22: ffff0000f5c93000
>>>>> [  315.802674] x21: ffff0000f4095400 x20: ffff0000f86b5540
>>>>> [  315.807975] x19: 0000000000000000 x18: 0000000000000000
>>>>> [  315.813276] x17: 0000000000000001 x16: 0000000000000019
>>>>> [  315.818578] x15: 000000148ccdabe2 x14: 0000000000000136
>>>>> [  315.823879] x13: 0000000000000001 x12: 00000000000003f8
>>>>> [  315.829180] x11: 0000000000000000 x10: 0000000000000000
>>>>> [  315.834482] x9 : ffff0000ff899990 x8 : ffff0000ff899000
>>>>> [  315.839784] x7 : 0000000040000000 x6 : 0000000000210d00
>>>>> [  315.845085] x5 : 0000000000000001 x4 : 0000000000000000
>>>>> [  315.850386] x3 : 00000000000000b0 x2 : 0000000000000001
>>>>> [  315.855687] x1 : 0000000000000000 x0 : 0000000000000001
>>>>> [  315.860988] Call trace:
>>>>> [  315.863432]  _raw_write_lock_irqsave+0xb0/0x2b8
>>>>> [  315.867956]  ida_free+0x48/0x158
>>>>> [  315.871184]  __media_device_unregister_entity+0x28/0xf0
>>>>> [  315.876402]  media_device_unregister+0x6c/0x148
>>>>> [  315.880927]  host1x_video_remove+0x20/0x48
>>>>> [  315.885021]  host1x_device_remove+0x1c/0x30
>>>>> [  315.889198]  device_release_driver_internal+0xf4/0x1c0
>>>>> [  315.894325]  device_driver_detach+0x14/0x20
>>>>> [  315.898503]  unbind_store+0xd4/0xf8
>>>>> [  315.901986]  drv_attr_store+0x20/0x30
>>>>> [  315.905645]  sysfs_kf_write+0x40/0x50
>>>>> [  315.909301]  kernfs_fop_write+0xf8/0x210
>>>>> [  315.913219]  __vfs_write+0x18/0x40
>>>>> [  315.916616]  vfs_write+0xdc/0x1c8
>>>>> [  315.919926]  ksys_write+0x68/0xf0
>>>>> [  315.923235]  __arm64_sys_write+0x18/0x20
>>>>> [  315.927154]  el0_svc_common.constprop.0+0x68/0x160
>>>>> [  315.931936]  do_el0_svc+0x20/0x80
>>>>> [  315.935246]  el0_sync_handler+0x10c/0x180
>>>>> [  315.939246]  el0_sync+0x140/0x180
>>>>> [  315.942560] Code: 8803fc02 35ffffa3 17fffda6 f9800071 (885ffc60)
>>>>> [  315.948644] ---[ end trace e42b943f3c1af06c ]---
>>>>>
>>>>> The following diff fixes this:
>>>>>
>>>>> ------------------ cut here ------------------
>>>>> diff --git a/drivers/staging/media/tegra/tegra-vi.c
>>>>> b/drivers/staging/media/tegra/tegra-vi.c
>>>>> index 9714152aa6a7..53cf37af9602 100644
>>>>> --- a/drivers/staging/media/tegra/tegra-vi.c
>>>>> +++ b/drivers/staging/media/tegra/tegra-vi.c
>>>>> @@ -583,7 +583,7 @@ static int tegra_channel_init(struct
>>>>> tegra_vi_channel *chan)
>>>>>         /* initialize the video_device */
>>>>>         chan->video->fops = &tegra_channel_fops;
>>>>>         chan->video->v4l2_dev = &vid->v4l2_dev;
>>>>> -     chan->video->release = video_device_release_empty;
>>>>> +     chan->video->release = video_device_release;
>>>>>         chan->video->queue = &chan->queue;
>>>>>         snprintf(chan->video->name, sizeof(chan->video->name),
>>>>> "%s-%s-%u",
>>>>>                  dev_name(vi->dev), "output", chan->portno);
>>>>> @@ -647,6 +647,7 @@ static int tegra_channel_init(struct
>>>>> tegra_vi_channel *chan)
>>>>>         media_entity_cleanup(&chan->video->entity);
>>>>>    release_vdev:
>>>>>         video_device_release(chan->video);
>>>>> +     chan->video = NULL;
>>>>>         return ret;
>>>>>    }
>>>>>
>>>>> @@ -707,7 +708,6 @@ static void tegra_vi_channels_cleanup(struct
>>>>> tegra_vi *vi)
>>>>>                         mutex_lock(&chan->video_lock);
>>>>>                         vb2_queue_release(&chan->queue);
>>>>>                         mutex_unlock(&chan->video_lock);
>>>>> -                     video_device_release(chan->video);
>>>>>                 }
>>>>>
>>>>>                 if (chan->frame_start_sp)
>>>>> ------------------ cut here ------------------
>>>> Note: Sakari suggested to embed struct video_device into struct
>>>> tegra_vi_channel.
>>>> In that case chan->video->release should remain
>>>> video_device_release_empty and
>>>> all video_device_alloc()/release() calls would have to be dropped.
>>> Thanks Hans. Tried several unbind/unbind not sure why it did not repro
>>> during my testing.
>>>
>>> video device is also part of tegra_vi_channel. So, v6 will remove
>>> video_device_alloc and use video_device_release_empty like I had in v3.
>>>
>>> This should help fix crash during unbind.
>>>
>>>> Regards,
>>>>
>>>>           Hans
>> With video device being part of the channel structure, it gets allocated
>> along with vi channel allocation during host1x vi client init and is
>> removed during channel free which happens during host1x vi client exit.
>>
>> Wit this during driver unbind with video node kept opened with v4l2-ctl
>> sleep, it crashes during media_device_unregister_entity.
>>
>> Using, separate video device allocation and freeing allocation during
>> video device release callback,  driver unbind works  as by the time of
>> entity unregister its it still available.
>>
>> So, will keep allocating video device separately with video_device_alloc
>> and will use video_device_release call back in v6.
>>
> Sounds good. Add a comment before video_device_alloc() explaining why it is
> used instead of embedding it in the vi channel data structure.
>
> Regards,
>
>          Hans

Other option I was thinking is to add tegra channel specific release 
callback and use that for video device release so video lock destroy and 
freeing channel free can be done in the last.

With this we don't need to do have separate video device allocation.