Re: [Intel-gfx] [RFC 12/14] drm/i915/pxp: User interface for Protected buffer
On 2/6/2021 4:25 AM, Chris Wilson wrote:
Quoting Daniele Ceraolo Spurio (2021-02-06 02:09:23)
From: Bommu Krishnaiah
This api allow user mode to create Protected buffer and context creation.
Only contexts created with the flag set are allowed to operate on
protected buffers.
We only allow setting the flags at creation time; the context flag also
requires the context to be marked as unrecoverable.
This is a rework + squash of the original code by Bommu Krishnaiah. I've
authorship unchanged since significant chunks have not been modified.
Signed-off-by: Bommu Krishnaiah
Signed-off-by: Daniele Ceraolo Spurio
Cc: Telukuntla Sreedhar
Cc: Kondapally Kalyan
Cc: Gupta Anshuman
Cc: Huang Sean Z
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 34 +++
drivers/gpu/drm/i915/gem/i915_gem_context.h | 6
.../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
drivers/gpu/drm/i915/gem/i915_gem_create.c| 27 +--
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 9 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 5 +++
drivers/gpu/drm/i915/pxp/intel_pxp.h | 10 ++
include/uapi/drm/i915_drm.h | 19 +++
8 files changed, 108 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ecacfae8412d..d3d9b4578ba8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -76,6 +76,8 @@
#include "gt/intel_gpu_commands.h"
#include "gt/intel_ring.h"
+#include "pxp/intel_pxp.h"
+
#include "i915_drm_client.h"
#include "i915_gem_context.h"
#include "i915_globals.h"
@@ -2006,6 +2008,27 @@ static int set_priority(struct i915_gem_context *ctx,
return 0;
}
+static int set_protected(struct i915_gem_context *ctx,
+const struct drm_i915_gem_context_param *args)
+{
+ int ret = 0;
+
+ if (ctx->client) /* can't change this after creation! */
+ ret = -EEXIST;
+ else if (args->size)
+ ret = -EINVAL;
+ else if (i915_gem_context_is_recoverable(ctx))
+ ret = -EPERM;
+ else if (!intel_pxp_is_enabled(>i915->gt.pxp))
+ ret = -ENODEV;
I like HW validity checks early. I think that gives a more consistent
response.
Ok, will do it first.
+ else if (args->value)
+ set_bit(UCONTEXT_PROTECTED, >user_flags);
+ else
+ clear_bit(UCONTEXT_PROTECTED, >user_flags);
+
+ return ret;
+}
+
static int ctx_setparam(struct drm_i915_file_private *fpriv,
struct i915_gem_context *ctx,
struct drm_i915_gem_context_param *args)
@@ -2045,6 +2068,8 @@ static int ctx_setparam(struct drm_i915_file_private
*fpriv,
case I915_CONTEXT_PARAM_RECOVERABLE:
if (args->size)
ret = -EINVAL;
+ else if (i915_gem_context_can_use_protected_content(ctx))
+ ret = -EPERM;
else if (args->value)
i915_gem_context_set_recoverable(ctx);
else
@@ -2075,6 +2100,10 @@ static int ctx_setparam(struct drm_i915_file_private
*fpriv,
ret = set_ringsize(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
+ ret = set_protected(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -2532,6 +2561,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device
*dev, void *data,
ret = get_ringsize(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
+ args->size = 0;
+ args->value = i915_gem_context_can_use_protected_content(ctx);
The getter should also report feature availability, i.e.
if (!intel_pxp_is_enabled(>i915->gt.pxp))
ret = -ENODEV;
else
args->value = i915_gem_context_can_use_protected_content(ctx);
Stick it in a get_protected_content() so it can sit next to the setter.
This allows userspace to do a feature query on an existing context (i.e.
the default context) without having to create anything [else]. For
example, that's useful for probing features sets once during screen setup.
ok
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index b5c908f3f4f2..473bce972bb2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -108,6 +108,12 @@ i915_gem_context_clear_user_engines(struct
i915_gem_context *ctx)
clear_bit(CONTEXT_USER_ENGINES, >flags);
}
+static inline bool
Re: [Intel-gfx] [RFC 12/14] drm/i915/pxp: User interface for Protected buffer
Quoting Daniele Ceraolo Spurio (2021-02-06 02:09:23)
> From: Bommu Krishnaiah
>
> This api allow user mode to create Protected buffer and context creation.
> Only contexts created with the flag set are allowed to operate on
> protected buffers.
>
> We only allow setting the flags at creation time; the context flag also
> requires the context to be marked as unrecoverable.
>
> This is a rework + squash of the original code by Bommu Krishnaiah. I've
> authorship unchanged since significant chunks have not been modified.
>
> Signed-off-by: Bommu Krishnaiah
> Signed-off-by: Daniele Ceraolo Spurio
> Cc: Telukuntla Sreedhar
> Cc: Kondapally Kalyan
> Cc: Gupta Anshuman
> Cc: Huang Sean Z
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 34 +++
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 6
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
> drivers/gpu/drm/i915/gem/i915_gem_create.c| 27 +--
> .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 9 +
> .../gpu/drm/i915/gem/i915_gem_object_types.h | 5 +++
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 10 ++
> include/uapi/drm/i915_drm.h | 19 +++
> 8 files changed, 108 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index ecacfae8412d..d3d9b4578ba8 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -76,6 +76,8 @@
> #include "gt/intel_gpu_commands.h"
> #include "gt/intel_ring.h"
>
> +#include "pxp/intel_pxp.h"
> +
> #include "i915_drm_client.h"
> #include "i915_gem_context.h"
> #include "i915_globals.h"
> @@ -2006,6 +2008,27 @@ static int set_priority(struct i915_gem_context *ctx,
> return 0;
> }
>
> +static int set_protected(struct i915_gem_context *ctx,
> +const struct drm_i915_gem_context_param *args)
> +{
> + int ret = 0;
> +
> + if (ctx->client) /* can't change this after creation! */
> + ret = -EEXIST;
> + else if (args->size)
> + ret = -EINVAL;
> + else if (i915_gem_context_is_recoverable(ctx))
> + ret = -EPERM;
> + else if (!intel_pxp_is_enabled(>i915->gt.pxp))
> + ret = -ENODEV;
I like HW validity checks early. I think that gives a more consistent
response.
> + else if (args->value)
> + set_bit(UCONTEXT_PROTECTED, >user_flags);
> + else
> + clear_bit(UCONTEXT_PROTECTED, >user_flags);
> +
> + return ret;
> +}
> +
> static int ctx_setparam(struct drm_i915_file_private *fpriv,
> struct i915_gem_context *ctx,
> struct drm_i915_gem_context_param *args)
> @@ -2045,6 +2068,8 @@ static int ctx_setparam(struct drm_i915_file_private
> *fpriv,
> case I915_CONTEXT_PARAM_RECOVERABLE:
> if (args->size)
> ret = -EINVAL;
> + else if (i915_gem_context_can_use_protected_content(ctx))
> + ret = -EPERM;
> else if (args->value)
> i915_gem_context_set_recoverable(ctx);
> else
> @@ -2075,6 +2100,10 @@ static int ctx_setparam(struct drm_i915_file_private
> *fpriv,
> ret = set_ringsize(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
> + ret = set_protected(ctx, args);
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> @@ -2532,6 +2561,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device
> *dev, void *data,
> ret = get_ringsize(ctx, args);
> break;
>
> + case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
> + args->size = 0;
> + args->value = i915_gem_context_can_use_protected_content(ctx);
The getter should also report feature availability, i.e.
if (!intel_pxp_is_enabled(>i915->gt.pxp))
ret = -ENODEV;
else
args->value = i915_gem_context_can_use_protected_content(ctx);
Stick it in a get_protected_content() so it can sit next to the setter.
This allows userspace to do a feature query on an existing context (i.e.
the default context) without having to create anything [else]. For
example, that's useful for probing features sets once during screen setup.
> + break;
> +
> case I915_CONTEXT_PARAM_BAN_PERIOD:
> default:
> ret = -EINVAL;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> index b5c908f3f4f2..473bce972bb2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
> @@ -108,6 +108,12 @@ i915_gem_context_clear_user_engines(struct
> i915_gem_context *ctx)
>
[Intel-gfx] [RFC 12/14] drm/i915/pxp: User interface for Protected buffer
From: Bommu Krishnaiah
This api allow user mode to create Protected buffer and context creation.
Only contexts created with the flag set are allowed to operate on
protected buffers.
We only allow setting the flags at creation time; the context flag also
requires the context to be marked as unrecoverable.
This is a rework + squash of the original code by Bommu Krishnaiah. I've
authorship unchanged since significant chunks have not been modified.
Signed-off-by: Bommu Krishnaiah
Signed-off-by: Daniele Ceraolo Spurio
Cc: Telukuntla Sreedhar
Cc: Kondapally Kalyan
Cc: Gupta Anshuman
Cc: Huang Sean Z
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 34 +++
drivers/gpu/drm/i915/gem/i915_gem_context.h | 6
.../gpu/drm/i915/gem/i915_gem_context_types.h | 1 +
drivers/gpu/drm/i915/gem/i915_gem_create.c| 27 +--
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 9 +
.../gpu/drm/i915/gem/i915_gem_object_types.h | 5 +++
drivers/gpu/drm/i915/pxp/intel_pxp.h | 10 ++
include/uapi/drm/i915_drm.h | 19 +++
8 files changed, 108 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ecacfae8412d..d3d9b4578ba8 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -76,6 +76,8 @@
#include "gt/intel_gpu_commands.h"
#include "gt/intel_ring.h"
+#include "pxp/intel_pxp.h"
+
#include "i915_drm_client.h"
#include "i915_gem_context.h"
#include "i915_globals.h"
@@ -2006,6 +2008,27 @@ static int set_priority(struct i915_gem_context *ctx,
return 0;
}
+static int set_protected(struct i915_gem_context *ctx,
+const struct drm_i915_gem_context_param *args)
+{
+ int ret = 0;
+
+ if (ctx->client) /* can't change this after creation! */
+ ret = -EEXIST;
+ else if (args->size)
+ ret = -EINVAL;
+ else if (i915_gem_context_is_recoverable(ctx))
+ ret = -EPERM;
+ else if (!intel_pxp_is_enabled(>i915->gt.pxp))
+ ret = -ENODEV;
+ else if (args->value)
+ set_bit(UCONTEXT_PROTECTED, >user_flags);
+ else
+ clear_bit(UCONTEXT_PROTECTED, >user_flags);
+
+ return ret;
+}
+
static int ctx_setparam(struct drm_i915_file_private *fpriv,
struct i915_gem_context *ctx,
struct drm_i915_gem_context_param *args)
@@ -2045,6 +2068,8 @@ static int ctx_setparam(struct drm_i915_file_private
*fpriv,
case I915_CONTEXT_PARAM_RECOVERABLE:
if (args->size)
ret = -EINVAL;
+ else if (i915_gem_context_can_use_protected_content(ctx))
+ ret = -EPERM;
else if (args->value)
i915_gem_context_set_recoverable(ctx);
else
@@ -2075,6 +2100,10 @@ static int ctx_setparam(struct drm_i915_file_private
*fpriv,
ret = set_ringsize(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
+ ret = set_protected(ctx, args);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
@@ -2532,6 +2561,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device
*dev, void *data,
ret = get_ringsize(ctx, args);
break;
+ case I915_CONTEXT_PARAM_PROTECTED_CONTENT:
+ args->size = 0;
+ args->value = i915_gem_context_can_use_protected_content(ctx);
+ break;
+
case I915_CONTEXT_PARAM_BAN_PERIOD:
default:
ret = -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.h
b/drivers/gpu/drm/i915/gem/i915_gem_context.h
index b5c908f3f4f2..473bce972bb2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.h
@@ -108,6 +108,12 @@ i915_gem_context_clear_user_engines(struct
i915_gem_context *ctx)
clear_bit(CONTEXT_USER_ENGINES, >flags);
}
+static inline bool
+i915_gem_context_can_use_protected_content(const struct i915_gem_context *ctx)
+{
+ return test_bit(UCONTEXT_PROTECTED, >user_flags);
+}
+
/* i915_gem_context.c */
void i915_gem_init__contexts(struct drm_i915_private *i915);
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 085f6a3735e8..1cab741983c9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -130,6 +130,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
