| Message ID | 11d8419744e4e744a9448180801b0c4683328afd.1597931876.git.robin.murphy@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 2020-08-20 17:53, Sakari Ailus wrote: > Hi Robin, > > On Thu, Aug 20, 2020 at 04:08:36PM +0100, Robin Murphy wrote: >> Now that arch/arm is wired up for default domains and iommu-dma, devices >> behind IOMMUs will get mappings set up automatically as appropriate, so >> there is no need for drivers to do so manually. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > Thanks for the patch. Many thanks for testing so quickly! > I haven't looked at the details but it seems that this causes the buffer > memory allocation to be physically contiguous, which causes a failure to > allocate video buffers of entirely normal size. I guess that was not > intentional? Hmm, it looks like the device ends up with the wrong DMA ops, which implies something didn't go as expected with the earlier IOMMU setup and default domain creation. Chances are that either I missed some subtlety in the omap_iommu change, or I've fundamentally misjudged how the ISP probing works and it never actually goes down the of_iommu_configure() path in the first place. Do you get any messages from the IOMMU layer earlier on during boot? Robin. > -----------------8<--------------------------- > [ 218.934448] WARNING: CPU: 0 PID: 1994 at mm/page_alloc.c:4859 __alloc_pages_nodemask+0x9c/0xb1c > [ 218.943847] Modules linked in: omap3_isp videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common leds_as3645a smiapp v4l2_flash_led_class led_class_flash v4l2_fwnode smiapp_pll videodev leds_gpio mc led_class > [ 218.964660] CPU: 0 PID: 1994 Comm: yavta Not tainted 5.9.0-rc1-dirty #1818 > [ 218.972442] Hardware name: Generic OMAP36xx (Flattened Device Tree) > [ 218.978973] Backtrace: > [ 218.981842] [<c010bf90>] (dump_backtrace) from [<c010c350>] (show_stack+0x20/0x24) > [ 218.989715] r7:00000000 r6:00000009 r5:c08f03bc r4:c08f2fef > [ 218.995880] [<c010c330>] (show_stack) from [<c03d3328>] (dump_stack+0x28/0x30) > [ 219.003631] [<c03d3300>] (dump_stack) from [<c012e324>] (__warn+0x100/0x118) > [ 219.010955] r5:c08f03bc r4:00000000 > [ 219.014953] [<c012e224>] (__warn) from [<c012e6f4>] (warn_slowpath_fmt+0x84/0xa8) > [ 219.022949] r9:c0232090 r8:c08f03bc r7:c0b08a88 r6:00000009 r5:000012fb r4:00000000 > [ 219.031036] [<c012e674>] (warn_slowpath_fmt) from [<c0232090>] (__alloc_pages_nodemask+0x9c/0xb1c) > [ 219.040557] r9:c0185c3c r8:00000000 r7:010ec000 r6:00000000 r5:0000000d r4:00000000 > [ 219.048858] [<c0231ff4>] (__alloc_pages_nodemask) from [<c01108f0>] (__dma_alloc_buffer.constprop.14+0x3c/0x90) > [ 219.059570] r10:00000cc0 r9:c0185c3c r8:00000000 r7:010ec000 r6:0000000d r5:c0b08a88 > [ 219.067901] r4:00000cc0 > [ 219.070587] [<c01108b4>] (__dma_alloc_buffer.constprop.14) from [<c0110a6c>] (remap_allocator_alloc+0x34/0x7c) > [ 219.081207] r9:c0185c3c r8:00000247 r7:e6d7fb84 r6:010ec000 r5:c0b08a88 r4:00000001 > [ 219.089263] [<c0110a38>] (remap_allocator_alloc) from [<c010f4f4>] (__dma_alloc+0x124/0x21c) > [ 219.098236] r9:ed99fc10 r8:e69aa890 r7:00000000 r6:ffffffff r5:c0b08a88 r4:e6fdd680 > [ 219.106536] [<c010f3d0>] (__dma_alloc) from [<c010f69c>] (arm_dma_alloc+0x68/0x74) > [ 219.114654] r10:00000cc0 r9:c0185c3c r8:00000cc0 r7:e69aa890 r6:010ec000 r5:ed99fc10 > [ 219.122985] r4:00000000 > [ 219.125671] [<c010f634>] (arm_dma_alloc) from [<c0185c3c>] (dma_alloc_attrs+0xe4/0x120) > [ 219.134216] r9:00000000 r8:e69aa890 r7:010ec000 r6:c0b08a88 r5:ed99fc10 r4:c010f634 > [ 219.142517] [<c0185b58>] (dma_alloc_attrs) from [<bf095c3c>] (vb2_dc_alloc+0xcc/0x108 [videobuf2_dma_contig]) > [ 219.153076] r10:e6885ca8 r9:e6abfc48 r8:00000002 r7:00000000 r6:010ec000 r5:ed99fc10 > [ 219.161407] r4:e69aa880 > [ 219.164184] [<bf095b70>] (vb2_dc_alloc [videobuf2_dma_contig]) from [<bf080fd0>] (__vb2_queue_alloc+0x258/0x4a4 [videobuf2_common]) > [ 219.176696] r8:bf095b70 r7:010ec000 r6:00000000 r5:e6885ca8 r4:e6abfc00 > [ 219.183959] [<bf080d78>] (__vb2_queue_alloc [videobuf2_common]) from [<bf0833a0>] (vb2_core_reqbufs+0x408/0x498 [videobuf2_common]) > [ 219.196533] r10:e6885ce8 r9:00000000 r8:e6d7fe24 r7:e6d7fcec r6:bf09ced4 r5:bf088580 > [ 219.204895] r4:e6885ca8 > [ 219.207672] [<bf082f98>] (vb2_core_reqbufs [videobuf2_common]) from [<bf08e1cc>] (vb2_reqbufs+0x64/0x70 [videobuf2_v4l2]) > [ 219.219268] r10:00000000 r9:bf032bc0 r8:c0145608 r7:bf0ad4a4 r6:e6885ca8 r5:00000000 > [ 219.227600] r4:e6d7fe24 > [ 219.230499] [<bf08e168>] (vb2_reqbufs [videobuf2_v4l2]) from [<bf09d7b4>] (isp_video_reqbufs+0x40/0x54 [omap3_isp]) > [ 219.241607] r7:bf0ad4a4 r6:e6d7fe24 r5:e6885c00 r4:e6cca928 > [ 219.247924] [<bf09d774>] (isp_video_reqbufs [omap3_isp]) from [<bf01de4c>] (v4l_reqbufs+0x4c/0x50 [videodev]) > [ 219.258514] r7:bf0ad4a4 r6:e6885c00 r5:e6d7fe24 r4:e7efbec0 > [ 219.264984] [<bf01de00>] (v4l_reqbufs [videodev]) from [<bf01eeb4>] (__video_do_ioctl+0x2d8/0x414 [videodev]) > [ 219.275512] r7:bf01de00 r6:00000000 r5:00000000 r4:e6cca2e0 > [ 219.281982] [<bf01ebdc>] (__video_do_ioctl [videodev]) from [<bf01fa1c>] (video_usercopy+0x144/0x508 [videodev]) > [ 219.292816] r10:e7efbec0 r9:c0145608 r8:e6d7fe24 r7:00000000 r6:00000000 r5:bf01ebdc > [ 219.300933] r4:c0145608 > [ 219.304168] [<bf01f8d8>] (video_usercopy [videodev]) from [<bf01fdfc>] (video_ioctl2+0x1c/0x24 [videodev]) > [ 219.314453] r10:e7fbfda0 r9:e7efbec0 r8:00000003 r7:00000000 r6:bee658f4 r5:c0145608 > [ 219.322784] r4:e7efbec0 > [ 219.325775] [<bf01fde0>] (video_ioctl2 [videodev]) from [<bf01814c>] (v4l2_ioctl+0x50/0x64 [videodev]) > [ 219.335845] [<bf0180fc>] (v4l2_ioctl [videodev]) from [<c02654a0>] (vfs_ioctl+0x30/0x44) > [ 219.344482] r7:00000000 r6:e7efbec0 r5:bee658f4 r4:c0145608 > [ 219.350402] [<c0265470>] (vfs_ioctl) from [<c0265e9c>] (sys_ioctl+0xdc/0x7ec) > [ 219.358062] [<c0265dc0>] (sys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > [ 219.366149] Exception stack(0xe6d7ffa8 to 0xe6d7fff0) > [ 219.371673] ffa0: 00000000 bee65c1a 00000003 c0145608 bee658f4 00000001 > [ 219.380157] ffc0: 00000000 bee65c1a 00000000 00000036 000009a0 00000000 0000ef30 010eb400 > [ 219.388885] ffe0: 0001716c bee65104 0000b588 b6e413ac > [ 219.394409] r10:00000036 r9:e6d7e000 r8:c0100244 r7:00000036 r6:00000000 r5:bee65c1a > [ 219.402740] r4:00000000 > [ 219.405426] irq event stamp: 5075 > [ 219.408905] hardirqs last enabled at (5083): [<c01778b0>] console_unlock+0x4cc/0x524 > [ 219.417297] hardirqs last disabled at (5092): [<c01777ac>] console_unlock+0x3c8/0x524 > [ 219.425628] softirqs last enabled at (4532): [<c01017d8>] __do_softirq+0x1f0/0x490 > [ 219.433837] softirqs last disabled at (4493): [<c0132c20>] irq_exit+0xe4/0x160 > [ 219.441558] ---[ end trace 8c56810633cf24db ]--- > [ 219.446502] omap3isp 480bc000.isp: dma_alloc_coherent of size 17743872 failed > -----------------8<--------------------------- >
On Thu, Aug 20, 2020 at 06:25:19PM +0100, Robin Murphy wrote: > On 2020-08-20 17:53, Sakari Ailus wrote: > > Hi Robin, > > > > On Thu, Aug 20, 2020 at 04:08:36PM +0100, Robin Murphy wrote: > > > Now that arch/arm is wired up for default domains and iommu-dma, devices > > > behind IOMMUs will get mappings set up automatically as appropriate, so > > > there is no need for drivers to do so manually. > > > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > > > > Thanks for the patch. > > Many thanks for testing so quickly! > > > I haven't looked at the details but it seems that this causes the buffer > > memory allocation to be physically contiguous, which causes a failure to > > allocate video buffers of entirely normal size. I guess that was not > > intentional? > > Hmm, it looks like the device ends up with the wrong DMA ops, which implies > something didn't go as expected with the earlier IOMMU setup and default > domain creation. Chances are that either I missed some subtlety in the > omap_iommu change, or I've fundamentally misjudged how the ISP probing works > and it never actually goes down the of_iommu_configure() path in the first > place. Do you get any messages from the IOMMU layer earlier on during boot? I do get these: [ 2.934936] iommu: Default domain type: Translated [ 2.940917] omap-iommu 480bd400.mmu: 480bd400.mmu registered [ 2.946899] platform 480bc000.isp: Adding to iommu group 0
On 8/20/20 6:01 PM, Robin Murphy wrote: > On 2020-08-20 20:55, Sakari Ailus wrote: >> On Thu, Aug 20, 2020 at 06:25:19PM +0100, Robin Murphy wrote: >>> On 2020-08-20 17:53, Sakari Ailus wrote: >>>> Hi Robin, >>>> >>>> On Thu, Aug 20, 2020 at 04:08:36PM +0100, Robin Murphy wrote: >>>>> Now that arch/arm is wired up for default domains and iommu-dma, devices >>>>> behind IOMMUs will get mappings set up automatically as appropriate, so >>>>> there is no need for drivers to do so manually. >>>>> >>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> >>>> Thanks for the patch. >>> >>> Many thanks for testing so quickly! >>> >>>> I haven't looked at the details but it seems that this causes the buffer >>>> memory allocation to be physically contiguous, which causes a failure to >>>> allocate video buffers of entirely normal size. I guess that was not >>>> intentional? >>> >>> Hmm, it looks like the device ends up with the wrong DMA ops, which implies >>> something didn't go as expected with the earlier IOMMU setup and default >>> domain creation. Chances are that either I missed some subtlety in the >>> omap_iommu change, or I've fundamentally misjudged how the ISP probing works >>> and it never actually goes down the of_iommu_configure() path in the first >>> place. Do you get any messages from the IOMMU layer earlier on during boot? Yeah, I don't think we go through the of_iommu_configure() path, the setup is mostly done using .probe_device() and .attach_dev() ops. Since the MMUs are present directly in the respective sub-systems and relies on the sub-system clocking and power, the MMU itself is turned ON and enabled during .attach_dev(). regards Suman >> >> I do get these: >> >> [ 2.934936] iommu: Default domain type: Translated >> [ 2.940917] omap-iommu 480bd400.mmu: 480bd400.mmu registered >> [ 2.946899] platform 480bc000.isp: Adding to iommu group 0 >> > > So that much looks OK, if there are no obvious errors. Unfortunately there's no > easy way to tell exactly what of_iommu_configure() is doing (beyond enabling a > couple of vague debug messages). The first thing I'll do tomorrow is > double-check whether it's really working on my boards here, or whether I was > just getting lucky with CMA... (I assume you don't have CMA enabled if you're > ending up in remap_allocator_alloc()) > > Robin.
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index b91e472ee764..196522883231 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -56,10 +56,6 @@ #include <linux/sched.h> #include <linux/vmalloc.h> -#ifdef CONFIG_ARM_DMA_USE_IOMMU -#include <asm/dma-iommu.h> -#endif - #include <media/v4l2-common.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-device.h> @@ -1942,51 +1938,6 @@ static int isp_initialize_modules(struct isp_device *isp) return ret; } -static void isp_detach_iommu(struct isp_device *isp) -{ -#ifdef CONFIG_ARM_DMA_USE_IOMMU - arm_iommu_detach_device(isp->dev); - arm_iommu_release_mapping(isp->mapping); - isp->mapping = NULL; -#endif -} - -static int isp_attach_iommu(struct isp_device *isp) -{ -#ifdef CONFIG_ARM_DMA_USE_IOMMU - struct dma_iommu_mapping *mapping; - int ret; - - /* - * Create the ARM mapping, used by the ARM DMA mapping core to allocate - * VAs. This will allocate a corresponding IOMMU domain. - */ - mapping = arm_iommu_create_mapping(&platform_bus_type, SZ_1G, SZ_2G); - if (IS_ERR(mapping)) { - dev_err(isp->dev, "failed to create ARM IOMMU mapping\n"); - return PTR_ERR(mapping); - } - - isp->mapping = mapping; - - /* Attach the ARM VA mapping to the device. */ - ret = arm_iommu_attach_device(isp->dev, mapping); - if (ret < 0) { - dev_err(isp->dev, "failed to attach device to VA mapping\n"); - goto error; - } - - return 0; - -error: - arm_iommu_release_mapping(isp->mapping); - isp->mapping = NULL; - return ret; -#else - return -ENODEV; -#endif -} - /* * isp_remove - Remove ISP platform device * @pdev: Pointer to ISP platform device @@ -2002,10 +1953,6 @@ static int isp_remove(struct platform_device *pdev) isp_cleanup_modules(isp); isp_xclk_cleanup(isp); - __omap3isp_get(isp, false); - isp_detach_iommu(isp); - __omap3isp_put(isp, false); - media_entity_enum_cleanup(&isp->crashed); v4l2_async_notifier_cleanup(&isp->notifier); @@ -2383,18 +2330,11 @@ static int isp_probe(struct platform_device *pdev) isp->mmio_hist_base_phys = mem->start + isp_res_maps[m].offset[OMAP3_ISP_IOMEM_HIST]; - /* IOMMU */ - ret = isp_attach_iommu(isp); - if (ret < 0) { - dev_err(&pdev->dev, "unable to attach to IOMMU\n"); - goto error_isp; - } - /* Interrupt */ ret = platform_get_irq(pdev, 0); if (ret <= 0) { ret = -ENODEV; - goto error_iommu; + goto error_isp; } isp->irq_num = ret; @@ -2402,13 +2342,13 @@ static int isp_probe(struct platform_device *pdev) "OMAP3 ISP", isp)) { dev_err(isp->dev, "Unable to request IRQ\n"); ret = -EINVAL; - goto error_iommu; + goto error_isp; } /* Entities */ ret = isp_initialize_modules(isp); if (ret < 0) - goto error_iommu; + goto error_isp; ret = isp_register_entities(isp); if (ret < 0) @@ -2433,8 +2373,6 @@ static int isp_probe(struct platform_device *pdev) isp_unregister_entities(isp); error_modules: isp_cleanup_modules(isp); -error_iommu: - isp_detach_iommu(isp); error_isp: isp_xclk_cleanup(isp); __omap3isp_put(isp, false); diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h index a9d760fbf349..b50459106d89 100644 --- a/drivers/media/platform/omap3isp/isp.h +++ b/drivers/media/platform/omap3isp/isp.h @@ -145,7 +145,6 @@ struct isp_xclk { * @syscon: Regmap for the syscon register space * @syscon_offset: Offset of the CSIPHY control register in syscon * @phy_type: ISP_PHY_TYPE_{3430,3630} - * @mapping: IOMMU mapping * @stat_lock: Spinlock for handling statistics * @isp_mutex: Mutex for serializing requests to ISP. * @stop_failure: Indicates that an entity failed to stop. @@ -185,8 +184,6 @@ struct isp_device { u32 syscon_offset; u32 phy_type; - struct dma_iommu_mapping *mapping; - /* ISP Obj */ spinlock_t stat_lock; /* common lock for statistic drivers */ struct mutex isp_mutex; /* For handling ref_count field */
Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/media/platform/omap3isp/isp.c | 68 ++------------------------- drivers/media/platform/omap3isp/isp.h | 3 -- 2 files changed, 3 insertions(+), 68 deletions(-)