[Intel-gfx] [PATCH v2 13/16] drm/i915/pxp: User interface for Protected buffer
Chris Wilson
chris at chris-wilson.co.uk
Wed Mar 3 23:33:58 UTC 2021
Quoting Daniele Ceraolo Spurio (2021-03-01 19:31:57)
> From: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
>
> This api allow user mode to create Protected buffers. Only contexts
> marked as protected are allowed to operate on protected buffers.
>
> We only allow setting the flags at creation time.
>
> All protected objects that have backing storage will be considered
> invalid when the session is destroyed and they won't be usable anymore.
>
> This is a rework of the original code by Bommu Krishnaiah. I've
> authorship unchanged since significant chunks have not been modified.
>
> v2: split context changes, fix defines and improve documentation (Chris),
> add object invalidation logic
>
> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Telukuntla Sreedhar <sreedhar.telukuntla at intel.com>
> Cc: Kondapally Kalyan <kalyan.kondapally at intel.com>
> Cc: Gupta Anshuman <Anshuman.Gupta at intel.com>
> Cc: Huang Sean Z <sean.z.huang at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_create.c | 27 +++++++++++--
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 10 +++++
> drivers/gpu/drm/i915/gem/i915_gem_object.c | 6 +++
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 12 ++++++
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 13 ++++++
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 40 +++++++++++++++++++
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 13 ++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 5 +++
> include/uapi/drm/i915_drm.h | 22 ++++++++++
> 9 files changed, 145 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> index 3ad3413c459f..d02e5938afbe 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
> @@ -5,6 +5,7 @@
>
> #include "gem/i915_gem_ioctls.h"
> #include "gem/i915_gem_region.h"
> +#include "pxp/intel_pxp.h"
>
> #include "i915_drv.h"
> #include "i915_user_extensions.h"
> @@ -13,7 +14,8 @@ static int
> i915_gem_create(struct drm_file *file,
> struct intel_memory_region *mr,
> u64 *size_p,
> - u32 *handle_p)
> + u32 *handle_p,
> + u64 user_flags)
> {
> struct drm_i915_gem_object *obj;
> u32 handle;
> @@ -35,12 +37,17 @@ i915_gem_create(struct drm_file *file,
>
> GEM_BUG_ON(size != obj->base.size);
>
> + obj->user_flags = user_flags;
> +
> ret = drm_gem_handle_create(file, &obj->base, &handle);
> /* drop reference from allocate - handle holds it now */
> i915_gem_object_put(obj);
> if (ret)
> return ret;
>
> + if (user_flags & I915_GEM_OBJECT_PROTECTED)
> + intel_pxp_object_add(obj);
> +
> *handle_p = handle;
> *size_p = size;
> return 0;
> @@ -89,11 +96,12 @@ i915_gem_dumb_create(struct drm_file *file,
> return i915_gem_create(file,
> intel_memory_region_by_type(to_i915(dev),
> mem_type),
> - &args->size, &args->handle);
> + &args->size, &args->handle, 0);
> }
>
> struct create_ext {
> struct drm_i915_private *i915;
> + unsigned long user_flags;
> };
>
> static int __create_setparam(struct drm_i915_gem_object_param *args,
> @@ -104,6 +112,19 @@ static int __create_setparam(struct drm_i915_gem_object_param *args,
> return -EINVAL;
> }
>
> + switch (lower_32_bits(args->param)) {
> + case I915_OBJECT_PARAM_PROTECTED_CONTENT:
> + if (!intel_pxp_is_enabled(&ext_data->i915->gt.pxp))
> + return -ENODEV;
> + if (args->size) {
> + return -EINVAL;
> + } else if (args->data) {
> + ext_data->user_flags |= I915_GEM_OBJECT_PROTECTED;
> + return 0;
> + }
> + break;
> + }
> +
> return -EINVAL;
> }
>
> @@ -148,5 +169,5 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
> return i915_gem_create(file,
> intel_memory_region_by_type(i915,
> INTEL_MEMORY_SYSTEM),
> - &args->size, &args->handle);
> + &args->size, &args->handle, ext_data.user_flags);
> }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index e503c9f789c0..d10c4fcb6aec 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -20,6 +20,7 @@
> #include "gt/intel_gt_buffer_pool.h"
> #include "gt/intel_gt_pm.h"
> #include "gt/intel_ring.h"
> +#include "pxp/intel_pxp.h"
>
> #include "pxp/intel_pxp.h"
>
> @@ -120,6 +123,9 @@ static void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *f
> struct i915_lut_handle *lut, *ln;
> LIST_HEAD(close);
>
> + if (i915_gem_object_has_valid_protection(obj))
> + intel_pxp_object_remove(obj);
This is called once for every handle in every fd, so a shared object will
be closed multiple times.
> @@ -500,6 +501,15 @@ eb_validate_vma(struct i915_execbuffer *eb,
> entry->offset != gen8_canonical_addr(entry->offset & I915_GTT_PAGE_MASK)))
> return -EINVAL;
>
> + if (i915_gem_object_is_protected(vma->obj)) {
Oh no, please not more cacheline to chase. Look to the vma. That also
helps with the problem of shared objects as you can check the closure of
the lut (i.e. the local reference of whether the protected vma is being used).
The bigger question is why though. This is to prevent reuse of protected
buffers in a new protected content context (because the earlier
invalidate would kill the previous context). In which case, you would
only need to do this check on new handles used by this context.
That also puts it in the position where the vma->obj has to be chased
anyway.
> + if (!intel_pxp_is_active(&vma->vm->gt->pxp))
> + return -ENODEV;
> + if (!i915_gem_object_has_valid_protection(vma->obj))
> + return -EIO;
> + if (!i915_gem_context_can_use_protected_content(eb->gem_context))
> + return -EPERM;
> + }
> +int intel_pxp_object_add(struct drm_i915_gem_object *obj)
> +{
> + struct intel_pxp *pxp = &to_i915(obj->base.dev)->gt.pxp;
> +
> + if (!intel_pxp_is_enabled(pxp))
> + return -ENODEV;
> +
> + if (!list_empty(&obj->pxp_link))
> + return -EEXIST;
> +
> + spin_lock_irq(&pxp->lock);
> + list_add(&obj->pxp_link, &pxp->protected_objects);
> + spin_unlock_irq(&pxp->lock);
> +
> + return 0;
> +}
> +
> +void intel_pxp_object_remove(struct drm_i915_gem_object *obj)
> +{
> + struct intel_pxp *pxp = &to_i915(obj->base.dev)->gt.pxp;
> +
> + if (!intel_pxp_is_enabled(pxp))
> + return;
> +
> + spin_lock_irq(&pxp->lock);
> + list_del_init(&obj->pxp_link);
> + spin_unlock_irq(&pxp->lock);
> +}
> +
> void intel_pxp_invalidate(struct intel_pxp *pxp)
> {
> struct drm_i915_private *i915 = pxp_to_gt(pxp)->i915;
> + struct drm_i915_gem_object *obj, *tmp;
> struct i915_gem_context *ctx, *cn;
>
> + /* delete objects that have been used with the invalidated session */
> + spin_lock_irq(&pxp->lock);
> + list_for_each_entry_safe(obj, tmp, &pxp->protected_objects, pxp_link) {
> + if (i915_gem_object_has_pages(obj))
> + list_del_init(&obj->pxp_link);
> + }
> + spin_unlock_irq(&pxp->lock);
pxp->lock is never used from irq context.
> @@ -28,6 +30,9 @@ struct intel_pxp {
> struct work_struct irq_work;
> bool irq_enabled;
> u32 current_events; /* protected with gt->irq_lock */
> +
> + struct spinlock lock;
struct spinlock is the odd one where spinlock_t is the preferred form.
More information about the Intel-gfx
mailing list