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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

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;