Nouveau - Aug 2020 - [PATCH] nouveau: fix the start/end range for migration

The user level OpenCL code shouldn't have to align start and end
addresses to a page boundary. That is better handled in the nouveau
driver. The npages field is also redundant since it can be computed
from the start and end addresses.

Signed-off-by: Ralph Campbell <rcampbell at nvidia.com>
---

This is for Ben Skegg's nouveau tree.

I have been working with Karol Herbst on the OpenCL mesa changes for
nouveau which will be merged upstream soon.
With or without those changes, the user visible effect of this patch
only extends the range by one page (round up vs. round down to page
boundary).

 drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
index 2df1c0460559..888aa0908c5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_svm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
@@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 	struct nouveau_cli *cli = nouveau_cli(file_priv);
 	struct drm_nouveau_svm_bind *args = data;
 	unsigned target, cmd, priority;
-	unsigned long addr, end, size;
+	unsigned long addr, end;
 	struct mm_struct *mm;
 
 	args->va_start &= PAGE_MASK;
-	args->va_end &= PAGE_MASK;
+	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
+	/* If no end address is given, assume a single page. */
+	if (args->va_end == 0)
+		args->va_end = args->va_start + PAGE_SIZE;
 
 	/* Sanity check arguments */
 	if (args->reserved0 || args->reserved1)
@@ -118,8 +121,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 		return -EINVAL;
 	if (args->va_start >= args->va_end)
 		return -EINVAL;
-	if (!args->npages)
-		return -EINVAL;
 
 	cmd = args->header >> NOUVEAU_SVM_BIND_COMMAND_SHIFT;
 	cmd &= NOUVEAU_SVM_BIND_COMMAND_MASK;
@@ -151,12 +152,6 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 	if (args->stride)
 		return -EINVAL;
 
-	size = ((unsigned long)args->npages) << PAGE_SHIFT;
-	if ((args->va_start + size) <= args->va_start)
-		return -EINVAL;
-	if ((args->va_start + size) > args->va_end)
-		return -EINVAL;
-
 	/*
 	 * Ok we are ask to do something sane, for now we only support migrate
 	 * commands but we will add things like memory policy (what to do on
@@ -171,7 +166,7 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	for (addr = args->va_start, end = args->va_start + size; addr < end;)
{
+	for (addr = args->va_start, end = args->va_end; addr < end;) {
 		struct vm_area_struct *vma;
 		unsigned long next;
 
-- 
2.20.1

On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell
wrote:
> The user level OpenCL code shouldn't have to align start and end
> addresses to a page boundary. That is better handled in the nouveau
> driver. The npages field is also redundant since it can be computed
> from the start and end addresses.
> 
> Signed-off-by: Ralph Campbell <rcampbell at nvidia.com>
> 
> This is for Ben Skegg's nouveau tree.
> 
> I have been working with Karol Herbst on the OpenCL mesa changes for
> nouveau which will be merged upstream soon.
> With or without those changes, the user visible effect of this patch
> only extends the range by one page (round up vs. round down to page
> boundary).
> 
>  drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 2df1c0460559..888aa0908c5a 100644
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void *data,
>  	struct nouveau_cli *cli = nouveau_cli(file_priv);
>  	struct drm_nouveau_svm_bind *args = data;
>  	unsigned target, cmd, priority;
> -	unsigned long addr, end, size;
> +	unsigned long addr, end;
>  	struct mm_struct *mm;
>  
>  	args->va_start &= PAGE_MASK;
> -	args->va_end &= PAGE_MASK;
> +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
> +	/* If no end address is given, assume a single page. */
> +	if (args->va_end == 0)
> +		args->va_end = args->va_start + PAGE_SIZE;
That is really weird, how is it useful for the kernel to map a region
of unknown size and alignment to the GPU?

Jason

On 8/31/20 4:51 AM, Jason Gunthorpe wrote:
> On Thu, Aug 27, 2020 at 02:37:44PM -0700, Ralph Campbell wrote:
>> The user level OpenCL code shouldn't have to align start and end
>> addresses to a page boundary. That is better handled in the nouveau
>> driver. The npages field is also redundant since it can be computed
>> from the start and end addresses.
>>
>> Signed-off-by: Ralph Campbell <rcampbell at nvidia.com>
>>
>> This is for Ben Skegg's nouveau tree.
>>
>> I have been working with Karol Herbst on the OpenCL mesa changes for
>> nouveau which will be merged upstream soon.
>> With or without those changes, the user visible effect of this patch
>> only extends the range by one page (round up vs. round down to page
>> boundary).
>>
>>   drivers/gpu/drm/nouveau/nouveau_svm.c | 17 ++++++-----------
>>   1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> index 2df1c0460559..888aa0908c5a 100644
>> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
>> @@ -105,11 +105,14 @@ nouveau_svmm_bind(struct drm_device *dev, void
*data,
>>   	struct nouveau_cli *cli = nouveau_cli(file_priv);
>>   	struct drm_nouveau_svm_bind *args = data;
>>   	unsigned target, cmd, priority;
>> -	unsigned long addr, end, size;
>> +	unsigned long addr, end;
>>   	struct mm_struct *mm;
>>   
>>   	args->va_start &= PAGE_MASK;
>> -	args->va_end &= PAGE_MASK;
>> +	args->va_end = ALIGN(args->va_end, PAGE_SIZE);
>> +	/* If no end address is given, assume a single page. */
>> +	if (args->va_end == 0)
>> +		args->va_end = args->va_start + PAGE_SIZE;
> 
> That is really weird, how is it useful for the kernel to map a region
> of unknown size and alignment to the GPU?
> 
> Jason
I agree it is somewhat weird. The OpenCL 2.2 specification says that
clEnqueueSVMMigrateMem() takes an array of pointers and sizes (in bytes)
but the size is optional. There is no alignment required.
This "works" because the pointers have to be allocated with
clSVMAlloc()
and presumably, the implementation for clSVMAlloc()
keeps track of the length allocated and can fill that in if size is zero.
However, requiring all allocations to be made with clSVMAlloc() defeats the
goal of being able to use regular malloc() and mmap() allocations for OpenCL
implementations that support fine-grained system allocations.
(See https://github.com/KhronosGroup/OpenCL-Docs/issues/392)

So if the size isn't specified, the most logical choices are do nothing and
return OK, return an error, or assume that at least one byte is being migrated
and try migrate it.