Hi Landwerlin,
Thanks for your feedback, Let me check with the commit owner. And I will upload another reversion once it's done.
Hi Krishnaiah,
May I have your comment for this? Please let me know if there is new revision path thanks.
Best regards,
Sean
-----Original Message-----
From: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
Sent: Friday, December 4, 2020 6:24 AM
To: Huang, Sean Z <sean.z.huang@xxxxxxxxx>; Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Bommu, Krishnaiah <krishnaiah.bommu@xxxxxxxxx>; Kondapally, Kalyan <kalyan.kondapally@xxxxxxxxx>
Subject: Re: [RFC-v4 24/26] drm/i915/pxp: User interface for Protected buffer
On 02/12/2020 06:03, Huang, Sean Z wrote:
> From: Bommu Krishnaiah <krishnaiah.bommu@xxxxxxxxx>
>
> This api allow user mode to create Protected buffer and context creation.
>
> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@xxxxxxxxx>
> Cc: Telukuntla Sreedhar <sreedhar.telukuntla@xxxxxxxxx>
> Cc: Kondapally Kalyan <kalyan.kondapally@xxxxxxxxx>
> Cc: Gupta Anshuman <Anshuman.Gupta@xxxxxxxxx>
> Cc: Huang Sean Z <sean.z.huang@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 15 ++++++++++--
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 10 ++++++++
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 2 +-
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 5 ++++
> drivers/gpu/drm/i915/i915_gem.c | 23 +++++++++++++++----
> include/uapi/drm/i915_drm.h | 19 +++++++++++++++
> 6 files changed, 67 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 a6299da64de4..dd5d24a13cb9 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -2060,12 +2060,23 @@ static int ctx_setparam(struct drm_i915_file_private *fpriv,
> case I915_CONTEXT_PARAM_RECOVERABLE:
> if (args->size)
> ret = -EINVAL;
> - else if (args->value)
> - i915_gem_context_set_recoverable(ctx);
> + else if (args->value) {
> + if (!i915_gem_context_is_protected(ctx))
> + i915_gem_context_set_recoverable(ctx);
> + else
> + ret = -EPERM;
> + }
> else
> i915_gem_context_clear_recoverable(ctx);
> break;
>
> + case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
> + if (args->size)
> + ret = -EINVAL;
> + else if (args->value)
> + i915_gem_context_set_protected(ctx);
> + break;
> +
> case I915_CONTEXT_PARAM_PRIORITY:
> ret = set_priority(ctx, args);
> break;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index a133f92bbedb..5897e7ca11a8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -70,6 +70,16 @@ static inline void i915_gem_context_set_recoverable(struct i915_gem_context *ctx
> set_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags);
> }
>
> +static inline void i915_gem_context_set_protected(struct
> +i915_gem_context *ctx) {
> + set_bit(UCONTEXT_PROTECTED, &ctx->user_flags); }
> +
> +static inline bool i915_gem_context_is_protected(struct
> +i915_gem_context *ctx) {
> + return test_bit(UCONTEXT_PROTECTED, &ctx->user_flags); }
> +
> static inline void i915_gem_context_clear_recoverable(struct i915_gem_context *ctx)
> {
> clear_bit(UCONTEXT_RECOVERABLE, &ctx->user_flags); diff --git
> a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index ae14ca24a11f..81ae94c2be86 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -135,7 +135,7 @@ struct i915_gem_context {
> #define UCONTEXT_BANNABLE 2
> #define UCONTEXT_RECOVERABLE 3
> #define UCONTEXT_PERSISTENCE 4
> -
> +#define UCONTEXT_PROTECTED 5
> /**
> * @flags: small set of booleans
> */
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index e2d9b7e1e152..90ac955463f4 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -161,6 +161,11 @@ struct drm_i915_gem_object {
> } mmo;
>
> I915_SELFTEST_DECLARE(struct list_head st_link);
> + /**
> + * @user_flags: small set of booleans set by the user
> + */
> + unsigned long user_flags;
> +#define I915_BO_PROTECTED BIT(0)
>
> unsigned long flags;
> #define I915_BO_ALLOC_CONTIGUOUS BIT(0) diff --git
> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 41698a823737..6a791fd24eaa 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -184,7 +184,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;
> @@ -204,6 +205,8 @@ i915_gem_create(struct drm_file *file,
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> + 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);
> @@ -258,11 +261,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;
> + struct drm_i915_private *i915;
> + unsigned long user_flags;
> };
>
> static int __create_setparam(struct drm_i915_gem_object_param *args,
> @@ -273,6 +277,17 @@ static int __create_setparam(struct drm_i915_gem_object_param *args,
> return -EINVAL;
> }
>
> + switch (lower_32_bits(args->param)) {
> + case I915_PARAM_PROTECTED_CONTENT:
> + if (args->size) {
> + return -EINVAL;
> + } else if (args->data) {
> + ext_data->user_flags = args->data;
> + return 0;
> + }
> + break;
> + }
> +
> return -EINVAL;
> }
>
> @@ -318,7 +333,7 @@ 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);
> }
>
> static int
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 2c1ce2761d55..fab00bfbbdee 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1715,6 +1715,15 @@ struct drm_i915_gem_context_param {
> * Default is 16 KiB.
> */
> #define I915_CONTEXT_PARAM_RINGSIZE 0xc
> +
> +/*
> + * I915_CONTEXT_PARAM_PROTECTED_CONTENT:
> + *
> + * If set to true (1) PAVP content protection is enabled.
> + * When enabled, the context is marked unrecoverable and may
> + * become invalid due to PAVP teardown event or other error.
> + */
> +#define I915_CONTEXT_PARAM_PROTECTED_CONTENT 0xd
This is about protected contexts. It should probably go into its own patch along with the code for the gem_context object.
-Lionel
> /* Must be kept compact -- no holes and well documented */
>
> __u64 value;
> @@ -1734,6 +1743,16 @@ struct drm_i915_gem_object_param {
> */
> #define I915_OBJECT_PARAM (1ull<<32)
>
> +/*
> + * I915_PARAM_PROTECTED_CONTENT:
> + *
> + * If set to true (1) buffer contents is expected to be protected by
> + * PAVP encryption and requires decryption for scan out and processing.
> + * Protected buffers can only be used in PAVP protected contexts.
> + * A protected buffer may become invalid as a result of PAVP teardown.
> + */
> +#define I915_PARAM_PROTECTED_CONTENT 0x1
> +
> __u64 param;
>
> /* Data value or pointer */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
