On Sat, Jul 10, 2021 at 04:24:46PM -0500, Jason Ekstrand wrote:

Allowing setting it multiple times brings no real utility to the API, no userspace relies on it, and it does make i915 a tiny bit more complicated. Let's disallow it for now unless someone comes up with a compelling reason to support it.

Maybe mention this is for symmetry with other proto ctx set_param operations, like set_engines.

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 7d6f52d8a8012..5853737cc79f3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -319,7 +319,6 @@ static int set_proto_ctx_vm(struct drm_i915_file_private *fpriv, const struct drm_i915_gem_context_param *args) { struct drm_i915_private *i915 = fpriv->dev_priv;

• struct i915_address_space *vm;

  if (args->size) return -EINVAL;

@@ -327,17 +326,16 @@ static int set_proto_ctx_vm(struct drm_i915_file_private *fpriv, if (!HAS_FULL_PPGTT(i915)) return -ENODEV;

• if (pc->vm)

• return -EINVAL;
  

• if (upper_32_bits(args->value)) return -ENOENT;

• vm = i915_gem_vm_lookup(fpriv, args->value);
• if (!vm)

• pc->vm = i915_gem_vm_lookup(fpriv, args->value);
• if (!pc->vm) return -ENOENT;

• if (pc->vm)

• i915_vm_put(pc->vm);
  

• pc->vm = vm;
• return 0;

}

-- 2.31.1

On Sat, Jul 10, 2021 at 04:24:47PM -0500, Jason Ekstrand wrote:

Signed-off-by: Jason Ekstrand jason@jlekstrand.net Cc: Daniel Vetter daniel.vetter@ffwll.ch

---

include/uapi/drm/i915_drm.h | 39 ++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index e334a8b14ef2d..b6bfbda1258c8 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1676,15 +1676,25 @@ struct drm_i915_gem_context_param { */ #define I915_CONTEXT_PARAM_RECOVERABLE 0x8

• /*

• * The id of the associated virtual memory address space (ppGTT) of
  

• * this context. Can be retrieved and passed to another context
  

• * (on the same fd) for both to use the same ppGTT and so share
  

• * address layouts, and avoid reloading the page tables on context
  

• * switches between themselves.
  

• *
  

• * See DRM_I915_GEM_VM_CREATE and DRM_I915_GEM_VM_DESTROY.
  

• */
  

+/*

So the annoying thing here is that this way it's not picked up by the kerneldoc machinery.

I think what Matt has done is included parameters as an item list in the relevant struct kerneldoc, which seems like the most reasonable thing to do with them.

That means some mild duplication for get/set parts, but since we've made the uapi very asymmetric in that, that's probably a feature.

• • The id of the associated virtual memory address space (ppGTT) of
• • this context. Can be retrieved and passed to another context
• • (on the same fd) for both to use the same ppGTT and so share
• • address layouts, and avoid reloading the page tables on context
• • switches between themselves.
• • The VM id should only be set via I915_CONTEXT_CREATE_EXT_SETPARAM.
• • On GPUs with graphics version 12 and earlier, it may also be set via
• • DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM. However, this is only for
• • backwards compatibility with old userspace and should be considered
• • deprecated. Starting with graphics version 13, it can only be set via
• • I915_CONTEXT_CREATE_EXT_SETPARAM. When using setparam, it may only be
• • set once for each context and, once the context has been used with any
• • ioctl other than I915_CONTEXT_CREATE_EXT_SETPARAM, it may not be set.
• • See DRM_I915_GEM_VM_CREATE and DRM_I915_GEM_VM_DESTROY.

Again for kerneldoc linking to the struct of the ioctl is probably better, since then you get a link.

• */

#define I915_CONTEXT_PARAM_VM 0x9

/* @@ -1700,8 +1710,15 @@ struct drm_i915_gem_context_param {

• to specify a gap in the array that can be filled in later, e.g. by a
• virtual engine used for load balancing.

• • Setting the number of engines bound to the context to 0, by passing a zero
• • sized argument, will revert back to default settings.

• • The engine set should only be set via I915_CONTEXT_CREATE_EXT_SETPARAM.
• • On GPUs with graphics version 12 and earlier, it may also be set via
• • DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM. However, this is only for
• • backwards compatibility with old userspace and should be considered
• • deprecated. Starting with graphics version 13, it can only be set via
• • I915_CONTEXT_CREATE_EXT_SETPARAM. When using setparam, it may only be
• • set once for each context and, once the context has been used with any
• • ioctl other than I915_CONTEXT_CREATE_EXT_SETPARAM, it may not be set.
  • See struct i915_context_param_engines.

Maybe we should push most of the docs for params with structs into the struct's kerneldoc? -Daniel

-- 2.31.1