[PATCH v2] drm/i915/gt: Report full vm address range

[PATCH v2] drm/i915/gt: Report full vm address range

[Andi Shyti]

Commit 9bb66c179f50 ("drm/i915: Reserve some kernel space per
vm") has reserved an object for kernel space usage.

Userspace, though, needs to know the full address range.

In the former patch the reserved space was substructed from the
total amount of the VM space. Add it back when the user requests
the GTT size through ioctl (I915_CONTEXT_PARAM_GTT_SIZE).

Fixes: 9bb66c179f50 ("drm/i915: Reserve some kernel space per vm")
Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Michal Mrozek <michal.mrozek@intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Cc: <stable@vger.kernel.org> # v6.2+
Acked-by: Michal Mrozek <michal.mrozek@intel.com>
Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
Hi,

Just proposing a different implementation that doesn't affect
i915 internally but provides the same result. Instead of not
substracting the space during the reservation, I add it back 
during the ioctl call.

All the "vm->rsvd.vma->node.size" looks a bit ugly, but that's
how it is. Maybe a comment can help to understand better why
there is this addition.

I kept the Ack from Michal and Lionel, because the outcome from
userspace perspactive doesn't really change.

Andi

 drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 81f65cab1330..60d9e7fe33b3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -2454,7 +2454,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 	case I915_CONTEXT_PARAM_GTT_SIZE:
 		args->size = 0;
 		vm = i915_gem_context_get_eb_vm(ctx);
-		args->value = vm->total;
+		args->value = vm->total + vm->rsvd.vma->node.size;
 		i915_vm_put(vm);
 
 		break;
-- 
2.43.0

Re: [PATCH v2] drm/i915/gt: Report full vm address range

[Nirmoy Das]

Hi Andi,

On 3/21/2024 4:17 PM, Andi Shyti wrote:
> Commit 9bb66c179f50 ("drm/i915: Reserve some kernel space per
> vm") has reserved an object for kernel space usage.
>
> Userspace, though, needs to know the full address range.
>
> In the former patch the reserved space was substructed from the
> total amount of the VM space. Add it back when the user requests
> the GTT size through ioctl (I915_CONTEXT_PARAM_GTT_SIZE).
>
> Fixes: 9bb66c179f50 ("drm/i915: Reserve some kernel space per vm")
> Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Chris Wilson <chris.p.wilson@linux.intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Michal Mrozek <michal.mrozek@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> Cc: <stable@vger.kernel.org> # v6.2+
> Acked-by: Michal Mrozek <michal.mrozek@intel.com>
> Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
> Hi,
>
> Just proposing a different implementation that doesn't affect
> i915 internally but provides the same result. Instead of not
> substracting the space during the reservation, I add it back
> during the ioctl call.
>
> All the "vm->rsvd.vma->node.size" looks a bit ugly,

Yes, this need document and also vm->total should be vm->total and may 
be we should have

vm->usable which will be used by kernel internal and return vm->total.

For me, I am fine with the kernel change as long as UMD is aware/fine of 
side-effect if

UMD ended up using the reserved page. Basically we need to document this 
well :)

Also may be we should limit this reserving page only on platform where 
it is required ?


Regards,

Nirmoy

>   but that's
> how it is. Maybe a comment can help to understand better why
> there is this addition.
>
> I kept the Ack from Michal and Lionel, because the outcome from
> userspace perspactive doesn't really change.
>
> Andi
>
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 81f65cab1330..60d9e7fe33b3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -2454,7 +2454,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
>   	case I915_CONTEXT_PARAM_GTT_SIZE:
>   		args->size = 0;
>   		vm = i915_gem_context_get_eb_vm(ctx);
> -		args->value = vm->total;
> +		args->value = vm->total + vm->rsvd.vma->node.size;
>   		i915_vm_put(vm);
>   
>   		break;