Re: [PATCH v2 6/7] drm/panfrost: Add support for GPU heap allocations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux