On 25/07/2019 02:10, Rob Herring wrote:
> The midgard/bifrost GPUs need to allocate GPU heap memory which is
> allocated on GPU page faults and not pinned in memory. The vendor driver
> calls this functionality GROW_ON_GPF.
>
> This implementation assumes that BOs allocated with the
> PANFROST_BO_NOEXEC flag are never mmapped or exported. Both of those may
> actually work, but I'm unsure if there's some interaction there. It
> would cause the whole object to be pinned in memory which would defeat
> the point of this.
>
> On faults, we map in 2MB at a time in order to utilize huge pages (if
> enabled). Currently, once we've mapped pages in, they are only unmapped
> if the BO is freed. Once we add shrinker support, we can unmap pages
> with the shrinker.
I've taken this for a spin, unfortunately it falls over:
[ 599.733909] Unable to handle kernel NULL pointer dereference at
virtual address 0000000000000000
[ 599.742732] Mem abort info:
[ 599.745526] ESR = 0x96000046
[ 599.748612] Exception class = DABT (current EL), IL = 32 bits
[ 599.754559] SET = 0, FnV = 0
[ 599.757612] EA = 0, S1PTW = 0
[ 599.760780] Data abort info:
[ 599.763687] ISV = 0, ISS = 0x00000046
[ 599.767549] CM = 0, WnR = 1
[ 599.770546] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000af36d000
[ 599.777017] [0000000000000000] pgd=00000000af260003,
pud=00000000af269003, pmd=0000000000000000
[ 599.785782] Internal error: Oops: 96000046 [#1] SMP
[ 599.790667] Modules linked in: panfrost gpu_sched
[ 599.795390] CPU: 0 PID: 1007 Comm: irq/67-mmu Tainted: G S
5.3.0-rc1-00355-g8c4e5073a168-dirty #35
[ 599.805653] Hardware name: HiKey960 (DT)
[ 599.809577] pstate: 60000005 (nZCv daif -PAN -UAO)
[ 599.814384] pc : __sg_alloc_table+0x14/0x168
[ 599.818654] lr : sg_alloc_table+0x2c/0x60
[ 599.822660] sp : ffff000011bcbba0
[ 599.825973] x29: ffff000011bcbba0 x28: 0000000000000000
[ 599.831289] x27: ffff8000a87081e0 x26: 0000000000000000
[ 599.836603] x25: 0000000000000080 x24: 0000000000200000
[ 599.841917] x23: 0000000000000000 x22: 0000000000000000
[ 599.847231] x21: 0000000000000200 x20: 0000000000000000
[ 599.852546] x19: 00000000fffff000 x18: 0000000000000010
[ 599.857860] x17: 0000000000000000 x16: 0000000000000000
[ 599.863175] x15: ffffffffffffffff x14: 3030303030303030
[ 599.868489] x13: 3030303030302873 x12: 656761705f6d6f72
[ 599.873805] x11: 665f656c6261745f x10: 0000000000040000
[ 599.879118] x9 : 0000000000000000 x8 : ffff7e0000000000
[ 599.884434] x7 : ffff8000a870cff8 x6 : ffff0000104277e8
[ 599.889748] x5 : 0000000000000cc0 x4 : 0000000000000000
[ 599.895061] x3 : 0000000000000000 x2 : 0000000000000080
[ 599.900375] x1 : 0000000000000008 x0 : 0000000000000000
[ 599.905692] Call trace:
[ 599.908143] __sg_alloc_table+0x14/0x168
[ 599.912067] sg_alloc_table+0x2c/0x60
[ 599.915732] __sg_alloc_table_from_pages+0xd8/0x208
[ 599.920612] sg_alloc_table_from_pages+0x14/0x20
[ 599.925252] panfrost_mmu_map_fault_addr+0x288/0x368 [panfrost]
[ 599.931185] panfrost_mmu_irq_handler+0x134/0x298 [panfrost]
[ 599.936855] irq_thread_fn+0x28/0x78
[ 599.940435] irq_thread+0x124/0x1f8
[ 599.943931] kthread+0x120/0x128
[ 599.947166] ret_from_fork+0x10/0x18
[ 599.950753] Code: 7100009f 910003fd a9046bf9 1a821099 (a9007c1f)
[ 599.956854] ---[ end trace d99c6028af6bbbd8 ]---
It would appear that in the following call sgt==NULL:
> ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
> NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
Which means we've ended up with a BO with bo->sgt==NULL, bo->pages set
and bo->is_heap=true. My understanding is this should be impossible.
I haven't yet figured out how this happens - it seems to be just before
termination, so it might be a race with cleanup?
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Cc: Robin Murphy <robin.murphy@xxxxxxx>
> Cc: Steven Price <steven.price@xxxxxxx>
> Cc: Alyssa Rosenzweig <alyssa.rosenzweig@xxxxxxxxxxxxx>
> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> ---
> v2:
> - Stop leaking pages!
> - Properly call dma_unmap_sg on cleanup
> - Enforce PANFROST_BO_NOEXEC when PANFROST_BO_HEAP is set
>
> drivers/gpu/drm/panfrost/TODO | 2 -
> drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +-
> drivers/gpu/drm/panfrost/panfrost_gem.c | 23 +++-
> drivers/gpu/drm/panfrost/panfrost_gem.h | 8 ++
> drivers/gpu/drm/panfrost/panfrost_mmu.c | 134 ++++++++++++++++++++++--
> include/uapi/drm/panfrost_drm.h | 1 +
> 6 files changed, 159 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/TODO b/drivers/gpu/drm/panfrost/TODO
> index c2e44add37d8..64129bf73933 100644
> --- a/drivers/gpu/drm/panfrost/TODO
> +++ b/drivers/gpu/drm/panfrost/TODO
> @@ -14,8 +14,6 @@
> The hard part is handling when more address spaces are needed than what
> the h/w provides.
>
> -- Support pinning pages on demand (GPU page faults).
> -
> - Support userspace controlled GPU virtual addresses. Needed for Vulkan. (Tomeu)
>
> - Support for madvise and a shrinker.
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 7ebd82d8d570..46a6bec7a0f2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -50,7 +50,12 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> struct drm_panfrost_create_bo *args = data;
>
> if (!args->size || args->pad ||
> - (args->flags & ~PANFROST_BO_NOEXEC))
> + (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
> + return -EINVAL;
> +
> + /* Heaps should never be executable */
> + if ((args->flags & PANFROST_BO_HEAP) &&
> + !(args->flags & PANFROST_BO_NOEXEC))
> return -EINVAL;
>
> bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 63731f6c5223..1237fb531321 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -27,6 +27,17 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> drm_mm_remove_node(&bo->node);
> spin_unlock(&pfdev->mm_lock);
>
> + if (bo->sgts) {
> + int i;
> + int n_sgt = bo->base.base.size / SZ_2M;
> +
> + for (i = 0; i < n_sgt; i++) {
> + if (bo->sgts[i].sgl)
> + dma_unmap_sg(pfdev->dev, bo->sgts[i].sgl,
> + bo->sgts[i].nents, DMA_BIDIRECTIONAL);
> + }
> + }
> +
Here you're not freeing the memory for bo->sgts itself. I think there's
a missing "kfree(bo->sgts)".
Steve
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
