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

[Steven Price]

On 25/07/2019 22:28, Rob Herring wrote:
> On Thu, Jul 25, 2019 at 9:35 AM Steven Price <steven.price at arm.com> wrote:
>>
>> On 25/07/2019 15:59, Steven Price wrote:
>> [...]
>>> 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?
>>
>> That was a red herring - it's partly my test case doing something a bit
>> weird. This crash is caused by doing an mmap of a HEAP object before any
>> fault has occurred.
> 
> My brother Red is always causing problems. ;)
> 
> But you were right that I need to kfree bo->sgts. Additionally, I also
> need to call sg_free_table on each table.
> 
>> drm_gem_shmem_mmap() calls drm_gem_shmem_get_pages() which will populate
>> bo->base.pages (even if bo->is_heap).
>>
>> Either we should prevent mapping of HEAP objects, or alternatively
>> bo->base.pages could be allocated upfront instead of during the first
>> fault. My preference would be allocating it upfront because optimising
>> for the case of a HEAP BO which isn't used seems a bit weird. Although
>> there's still the question of exactly what the behaviour should be of
>> accessing through the CPU pages which haven't been allocated yet.
>>
>> Also shmem->pages_use_count needs incrementing to stop
>> drm_gem_shmem_get_pages() replacing bo->base.pages. I haven't tested
>> what happens if you mmap *after* the first fault.
> 
> I did say mmap had undefined/unknown behavior.

True - and I was surprised to find out my test was actually doing that! 
But crashing the kernel is perhaps a bit excessive!

Avoiding the mmap of HEAP objects everything runs fine - and the memory 
leaks are much smaller than they used to be :)

Steve