| Message ID | 20230821200201.24685-1-Philip.Yang@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/prime: Support page array >= 4GB | expand |
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. Christian. > > Signed-off-by: Philip Yang <Philip.Yang@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);
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). 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. Christian. > Regards, > > Philip > >> >> Christian. >> >>> >>> Signed-off-by: Philip Yang <Philip.Yang@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); >>
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@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); >>> >
On 2023-08-21 16:02, Philip Yang wrote: > 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. > > Signed-off-by: Philip Yang <Philip.Yang@amd.com> The patch looks reasonable to me. I don't have authority to approve it. But FWIW, Acked-by: Felix Kuehling <Felix.Kuehling@amd.com> Can anyone give a Reviewed-by? Thanks, Felix > --- > 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);
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);
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. Signed-off-by: Philip Yang <Philip.Yang@amd.com> --- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)