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

From: Sowjanya Komatineni
Date: Fri Apr 03 2020 - 01:45:15 EST


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.