[PATCH] drm/prime: Support page array >= 4GB

[Felix Kuehling]

On 2023-08-23 01:49, Christian König wrote:
> Am 22.08.23 um 20:27 schrieb Philip Yang:
>>
>> On 2023-08-22 05:43, Christian König wrote:
>>
>>>
>>> Am 21.08.23 um 22:02 schrieb Philip Yang:
>>>> Without unsigned long typecast, the size is passed in as zero if page
>>>> array size >= 4GB, nr_pages >= 0x100000, then sg list converted will
>>>> have the first and the last chunk lost.
>>>
>>> Good catch, but I'm not sure if this is enough to make it work.
>>>
>>> Additional to that I don't think we have an use case for BOs > 4GiB.
>>
>> >4GB buffer is normal for compute applications, the issue is reported 
>> by "Maelstrom generated exerciser detects micompares when GPU 
>> accesses larger remote GPU memory." on GFX 9.4.3 APU, which uses GTT 
>> domain to allocate VRAM, and trigger the bug in this drm prime 
>> helper. With this fix, the test passed.
>>
>
> Why is the application allocating all the data as a single BO?
>
> Usually you have a single texture, image, array etc... in a single BO 
> but this here looks a bit like the application tries to allocate all 
> their memory in a single BO (could of course be that this isn't the 
> case and that's really just one giant data structure).

Compute applications work with pretty big data structures. For example 
huge multi-dimensional matrices are not uncommon in large 
machine-learning models.


>
>
> Swapping such large BOs out at once is quite impractical, so should we 
> ever have an use case like suspend/resume or checkpoint/restore with 
> this it will most likely fail.
Checkpointing and restoring multiple GB at a time should not be a 
problem. I'm pretty sure we have tested that. On systems with 100s of 
GBs of memory, HBM memory bandwidth approaching TB/s and PCIe/CXL bus 
bandwidths going into 10s of GB/s, dealing with multi-GB BOs should not 
be a fundamental problem.

That said, if you wanted to impose limits on the size of single 
allocations, then I would expect some policy somewhere that prohibits 
large allocations. On the contrary, I see long or 64-bit data types all 
over the VRAM manager and TTM code, which tells me that >4GB allocations 
must be part of the plan.

This patch is clearly addressing a bug in the code that results in data 
corruption when mapping large BOs on multiple GPUs. You could address 
this with an allocation policy change, if you want, and leave the bug in 
place. Then we have to update ROCm user mode to break large allocations 
into multiple BOs. It would break applications that try to share such 
large allocations via DMABufs (e.g. with an RDMA NIC), because it would 
become impossible to share large allocations with a single DMABuf handle.

Regards,
   Felix


>
> Christian.
>
>> Regards,
>>
>> Philip
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Signed-off-by: Philip Yang <Philip.Yang at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_prime.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>> index f924b8b4ab6b..2630ad2e504d 100644
>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>> @@ -830,7 +830,7 @@ struct sg_table *drm_prime_pages_to_sg(struct 
>>>> drm_device *dev,
>>>>       if (max_segment == 0)
>>>>           max_segment = UINT_MAX;
>>>>       err = sg_alloc_table_from_pages_segment(sg, pages, nr_pages, 0,
>>>> -                        nr_pages << PAGE_SHIFT,
>>>> +                        (unsigned long)nr_pages << PAGE_SHIFT,
>>>>                           max_segment, GFP_KERNEL);
>>>>       if (err) {
>>>>           kfree(sg);
>>>
>