Re: [PATCH] drm/prime: Support page array >= 4GB
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 >= 0x10, then sg list converted will
have the first and the last chunk lost.
Signed-off-by: Philip Yang
The patch looks reasonable to me. I don't have authority to approve it.
But FWIW,
Acked-by: Felix Kuehling
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);
Re: [PATCH] drm/prime: Support page array >= 4GB
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 >= 0x10, 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
---
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);
Re: [PATCH] drm/prime: Support page array >= 4GB
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 >= 0x10, 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
---
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);
Re: [PATCH] drm/prime: Support page array >= 4GB
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 >= 0x10, 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.
Regards,
Philip
Christian.
Signed-off-by: Philip Yang
---
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);
Re: [PATCH] drm/prime: Support page array >= 4GB
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 >= 0x10, 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
---
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);
[PATCH] drm/prime: Support page array >= 4GB
Without unsigned long typecast, the size is passed in as zero if page
array size >= 4GB, nr_pages >= 0x10, then sg list converted will
have the first and the last chunk lost.
Signed-off-by: Philip Yang
---
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);
--
2.35.1
