Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Quoting Joonas Lahtinen (2020-11-16 10:22:23) > Quoting Huang, Sean Z (2020-11-15 23:07:53) > > Enable one ioctl action to allow ring3 driver to set its ring3 > > context, so ring0 PXP can track the context id through this ring3 > > context list. > > Overall the patches should refer to "userspace" not "ring3" to avoid > confusion. "kernel" vs "user" not ring0 vs ring3. There's also a missing chunk as to why this is not associated with the existing user context, rather than introducing a new incomplete encapsulation. Overall, you've left in an awful lot of debug code and failed to follow the coding style. A confusion as to whether your hw interactions is on the GT or the whole device (it's GT). Wrt the flow of the patches, robust setup and termination must be early in the series, not tacked onto the end. And you seem to confuse the kernel contexts as something special, you use them as non-privileged, just like ordinary userspace. Do not use the engine->kernel_context! You risk breaking (and judging from the waits you do add, it is inevitable that you have broken) power management and heartbeats. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Quoting Huang, Sean Z (2020-11-15 23:07:53)
> Enable one ioctl action to allow ring3 driver to set its ring3
> context, so ring0 PXP can track the context id through this ring3
> context list.
Overall the patches should refer to "userspace" not "ring3" to avoid
confusion. "kernel" vs "user" not ring0 vs ring3.
> Signed-off-by: Huang, Sean Z
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -8,6 +8,63 @@
> #include "intel_pxp_context.h"
> #include "intel_pxp_sm.h"
>
> +int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
> *drmfile)
> +{
> + int ret;
> + struct pxp_info pxp_info = {0};
> + struct drm_i915_pxp_ops *pxp_ops = data;
> + struct drm_i915_private *i915 = to_i915(dev);
> +
> + if (!i915 || !drmfile || !pxp_ops || pxp_ops->pxp_info_size !=
> sizeof(pxp_info))
> + return -EINVAL;
See below.
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -24,6 +24,21 @@ enum pxp_sm_session_req {
> PXP_SM_REQ_SESSION_TERMINATE
> };
>
> +#define PXP_ACTION_SET_R3_CONTEXT 5
> +
> +enum pxp_sm_status {
> + PXP_SM_STATUS_SUCCESS,
> + PXP_SM_STATUS_RETRY_REQUIRED,
> + PXP_SM_STATUS_SESSION_NOT_AVAILABLE,
> + PXP_SM_STATUS_ERROR_UNKNOWN
> +};
> +
> +struct pxp_info {
> + u32 action;
> + u32 sm_status;
> + u32 set_r3ctx;
> +} __packed;
These are part of the the uAPI, so it should all be in i915_drm.h. No
need to use pointers as that just adds extra layer of indirection. Every
added bit needs a link to an Open Source userspace implementation and
IGT tests.
After applying the IOCTL-per-action model there needs to be strict
rejection of any unrecognized bits in the structs to ensure userspace
does not grow to depend on being able to set bits which may be
re-purposed in the future.
R3_CONTEXT / set_r3ctx should be using the "user" vs "kernel" naming,
and once they are moved into the userspace API header, "user" becomes a
tautology.
All the fields added to uAPI need documentation, too.
> +++ b/include/uapi/drm/i915_drm.h
> @@ -359,6 +359,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_I915_QUERY 0x39
> #define DRM_I915_GEM_VM_CREATE 0x3a
> #define DRM_I915_GEM_VM_DESTROY0x3b
> +#define DRM_I915_PXP_OPS 0x3c
> /* Must be kept compact -- no holes */
>
> #define DRM_IOCTL_I915_INITDRM_IOW( DRM_COMMAND_BASE +
> DRM_I915_INIT, drm_i915_init_t)
> @@ -422,6 +423,7 @@ typedef struct _drm_i915_sarea {
> #define DRM_IOCTL_I915_QUERY DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_QUERY, struct drm_i915_query)
> #define DRM_IOCTL_I915_GEM_VM_CREATE DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_CREATE, struct drm_i915_gem_vm_control)
> #define DRM_IOCTL_I915_GEM_VM_DESTROY DRM_IOW (DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_DESTROY, struct drm_i915_gem_vm_control)
> +#define DRM_IOCTL_I915_PXP_OPS DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_PXP_OPS, struct drm_i915_pxp_ops)
Just to repeat: This should only be added in the last patches of the
series not to break bisecting. The different actions need to be spelled
out, a general purpose IOCTL can't be used.
Regards, Joonas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Hi Sean,
url:
https://github.com/0day-ci/linux/commits/Sean-Z-Huang/drm-i915-pxp-Introduce-Intel-PXP-component/20201114-094926
base:92edc4aef86780a8ad01b092c6d6630bb3cb423d
config: i386-randconfig-m021-20201115 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
smatch warnings:
drivers/gpu/drm/i915/pxp/intel_pxp.c:62 i915_pxp_ops_ioctl() error: we
previously assumed 'i915' could be null (see line 20)
vim +/i915 +62 drivers/gpu/drm/i915/pxp/intel_pxp.c
1d109ada10e82c3 Huang, Sean Z 2020-11-13 11 int i915_pxp_ops_ioctl(struct
drm_device *dev, void *data, struct drm_file *drmfile)
1d109ada10e82c3 Huang, Sean Z 2020-11-13 12 {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 13int ret;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 14struct pxp_info pxp_info = {0};
1d109ada10e82c3 Huang, Sean Z 2020-11-13 15struct drm_i915_pxp_ops
*pxp_ops = data;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 16struct drm_i915_private *i915 =
to_i915(dev);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 17
1d109ada10e82c3 Huang, Sean Z 2020-11-13 18drm_dbg(>drm, ">>> %s\n",
__func__);
^
If "i915" is NULL then this will crash.
1d109ada10e82c3 Huang, Sean Z 2020-11-13 19
1d109ada10e82c3 Huang, Sean Z 2020-11-13 @20if (!i915 || !drmfile ||
!pxp_ops || pxp_ops->pxp_info_size != sizeof(pxp_info)) {
Check too late.
1d109ada10e82c3 Huang, Sean Z 2020-11-13 21drm_dbg(>drm,
"Failed to %s, invalid params\n", __func__);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 22ret = -EINVAL;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 23goto end;
This will unlock a lock that we are not holding. This should just
"return -EINVAL;". All the stuff at "goto end;" is pointless or buggy.
1d109ada10e82c3 Huang, Sean Z 2020-11-13 24}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 25
1d109ada10e82c3 Huang, Sean Z 2020-11-13 26if (copy_from_user(_info,
(void __user *)pxp_ops->pxp_info_ptr, sizeof(pxp_info)) != 0) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 27ret = -EFAULT;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 28goto end;
This will unlock. Same. Just return -EFAULT;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 29}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 30
1d109ada10e82c3 Huang, Sean Z 2020-11-13 31drm_dbg(>drm, "i915 pxp
ioctl call with action=[%d]\n", pxp_info.action);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 32
1d109ada10e82c3 Huang, Sean Z 2020-11-13 33
mutex_lock(>pxp.r0ctx->ctx_mutex);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 34
1d109ada10e82c3 Huang, Sean Z 2020-11-13 35if
(i915->pxp.r0ctx->global_state_in_suspend) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 36drm_dbg(>drm,
"Return failure due to state in suspend\n");
1d109ada10e82c3 Huang, Sean Z 2020-11-13 37pxp_info.sm_status =
PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 38ret = 0;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 39goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 40}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 41
1d109ada10e82c3 Huang, Sean Z 2020-11-13 42if
(i915->pxp.r0ctx->global_state_attacked) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 43drm_dbg(>drm,
"Retry required due to state attacked\n");
1d109ada10e82c3 Huang, Sean Z 2020-11-13 44pxp_info.sm_status =
PXP_SM_STATUS_RETRY_REQUIRED;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 45ret = 0;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 46goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 47}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 48
1d109ada10e82c3 Huang, Sean Z 2020-11-13 49switch (pxp_info.action) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 50case PXP_ACTION_SET_R3_CONTEXT:
1d109ada10e82c3 Huang, Sean Z 2020-11-13 51{
1d109ada10e82c3 Huang, Sean Z 2020-11-13 52ret =
intel_pxp_set_r3ctx(i915, pxp_info.set_r3ctx);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 53break;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 54}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 55default:
1d109ada10e82c3 Huang, Sean Z 2020-11-13 56drm_dbg(>drm,
"Failed to %s due to bad params\n", __func__);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 57ret = -EINVAL;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 58goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 59}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 60
1d109ada10e82c3 Huang, Sean Z 2020-11-13 61 end:
1d109ada10e82c3 Huang, Sean Z 2020-11-13 @62
mutex_unlock(>pxp.r0ctx->ctx_mutex);
[Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Enable one ioctl action to allow ring3 driver to set its ring3
context, so ring0 PXP can track the context id through this ring3
context list.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 59
drivers/gpu/drm/i915/pxp/intel_pxp.h | 16 ++
drivers/gpu/drm/i915/pxp/intel_pxp_context.c | 34 +++
drivers/gpu/drm/i915/pxp/intel_pxp_context.h | 3 +
include/uapi/drm/i915_drm.h | 2 +
6 files changed, 115 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c8b9c42fcbd6..43ea85b5f14b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1761,6 +1761,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(I915_PXP_OPS, i915_pxp_ops_ioctl, DRM_RENDER_ALLOW),
};
static const struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 3a24c2b13b14..8fa88ea17bc0 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -8,6 +8,63 @@
#include "intel_pxp_context.h"
#include "intel_pxp_sm.h"
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile)
+{
+ int ret;
+ struct pxp_info pxp_info = {0};
+ struct drm_i915_pxp_ops *pxp_ops = data;
+ struct drm_i915_private *i915 = to_i915(dev);
+
+ if (!i915 || !drmfile || !pxp_ops || pxp_ops->pxp_info_size !=
sizeof(pxp_info))
+ return -EINVAL;
+
+ if (copy_from_user(_info, u64_to_user_ptr(pxp_ops->pxp_info_ptr),
sizeof(pxp_info)) != 0)
+ return -EFAULT;
+
+ drm_dbg(>drm, "i915 pxp ioctl call with action=[%d]\n",
pxp_info.action);
+
+ mutex_lock(>pxp.r0ctx->ctx_mutex);
+
+ if (i915->pxp.r0ctx->global_state_in_suspend) {
+ drm_dbg(>drm, "Return failure due to state in suspend\n");
+ pxp_info.sm_status = PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
+ ret = 0;
+ goto end;
+ }
+
+ if (i915->pxp.r0ctx->global_state_attacked) {
+ drm_dbg(>drm, "Retry required due to state attacked\n");
+ pxp_info.sm_status = PXP_SM_STATUS_RETRY_REQUIRED;
+ ret = 0;
+ goto end;
+ }
+
+ switch (pxp_info.action) {
+ case PXP_ACTION_SET_R3_CONTEXT:
+ {
+ ret = intel_pxp_set_r3ctx(i915, pxp_info.set_r3ctx);
+ break;
+ }
+ default:
+ drm_dbg(>drm, "Failed to %s due to bad params\n",
__func__);
+ ret = -EINVAL;
+ goto end;
+ }
+
+end:
+ mutex_unlock(>pxp.r0ctx->ctx_mutex);
+
+ if (ret == 0)
+ if (copy_to_user(u64_to_user_ptr(pxp_ops->pxp_info_ptr),
_info, sizeof(pxp_info)) != 0)
+ ret = -EFAULT;
+
+ if (ret)
+ dev_err(>pdev->dev, "pid=%d, ret = %d\n",
task_pid_nr(current), ret);
+
+ drm_dbg(>drm, "<<< %s\n", __func__);
+ return ret;
+}
+
static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
mask)
{
WARN_ON(INTEL_GEN(i915) < 11);
@@ -39,6 +96,8 @@ static int intel_pxp_teardown_required_callback(struct
drm_i915_private *i915)
i915->pxp.r0ctx->global_state_attacked = true;
i915->pxp.r0ctx->flag_display_hm_surface_keys = false;
+ intel_pxp_destroy_r3ctx_list(i915);
+
mutex_unlock(>pxp.r0ctx->ctx_mutex);
return 0;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 4dec35bb834d..21a6964fc64e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -24,6 +24,21 @@ enum pxp_sm_session_req {
PXP_SM_REQ_SESSION_TERMINATE
};
+#define PXP_ACTION_SET_R3_CONTEXT 5
+
+enum pxp_sm_status {
+ PXP_SM_STATUS_SUCCESS,
+ PXP_SM_STATUS_RETRY_REQUIRED,
+ PXP_SM_STATUS_SESSION_NOT_AVAILABLE,
+ PXP_SM_STATUS_ERROR_UNKNOWN
+};
+
+struct pxp_info {
+ u32 action;
+ u32 sm_status;
+ u32 set_r3ctx;
+} __packed;
+
struct pxp_context;
struct intel_pxp {
@@ -37,6 +52,7 @@ struct intel_pxp {
struct intel_gt;
struct drm_i915_private;
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile);
void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
int i915_pxp_teardown_required_callback(struct drm_i915_private *i915);
int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_context.c
[Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Enable one ioctl action to allow ring3 driver to set its ring3
context, so ring0 PXP can track the context id through this ring3
context list.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 59
drivers/gpu/drm/i915/pxp/intel_pxp.h | 16 ++
drivers/gpu/drm/i915/pxp/intel_pxp_context.c | 34 +++
drivers/gpu/drm/i915/pxp/intel_pxp_context.h | 3 +
include/uapi/drm/i915_drm.h | 2 +
6 files changed, 115 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c8b9c42fcbd6..43ea85b5f14b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1761,6 +1761,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(I915_PXP_OPS, i915_pxp_ops_ioctl, DRM_RENDER_ALLOW),
};
static const struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 3a24c2b13b14..8fa88ea17bc0 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -8,6 +8,63 @@
#include "intel_pxp_context.h"
#include "intel_pxp_sm.h"
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile)
+{
+ int ret;
+ struct pxp_info pxp_info = {0};
+ struct drm_i915_pxp_ops *pxp_ops = data;
+ struct drm_i915_private *i915 = to_i915(dev);
+
+ if (!i915 || !drmfile || !pxp_ops || pxp_ops->pxp_info_size !=
sizeof(pxp_info))
+ return -EINVAL;
+
+ if (copy_from_user(_info, u64_to_user_ptr(pxp_ops->pxp_info_ptr),
sizeof(pxp_info)) != 0)
+ return -EFAULT;
+
+ drm_dbg(>drm, "i915 pxp ioctl call with action=[%d]\n",
pxp_info.action);
+
+ mutex_lock(>pxp.r0ctx->ctx_mutex);
+
+ if (i915->pxp.r0ctx->global_state_in_suspend) {
+ drm_dbg(>drm, "Return failure due to state in suspend\n");
+ pxp_info.sm_status = PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
+ ret = 0;
+ goto end;
+ }
+
+ if (i915->pxp.r0ctx->global_state_attacked) {
+ drm_dbg(>drm, "Retry required due to state attacked\n");
+ pxp_info.sm_status = PXP_SM_STATUS_RETRY_REQUIRED;
+ ret = 0;
+ goto end;
+ }
+
+ switch (pxp_info.action) {
+ case PXP_ACTION_SET_R3_CONTEXT:
+ {
+ ret = intel_pxp_set_r3ctx(i915, pxp_info.set_r3ctx);
+ break;
+ }
+ default:
+ drm_dbg(>drm, "Failed to %s due to bad params\n",
__func__);
+ ret = -EINVAL;
+ goto end;
+ }
+
+end:
+ mutex_unlock(>pxp.r0ctx->ctx_mutex);
+
+ if (ret == 0)
+ if (copy_to_user(u64_to_user_ptr(pxp_ops->pxp_info_ptr),
_info, sizeof(pxp_info)) != 0)
+ ret = -EFAULT;
+
+ if (ret)
+ dev_err(>pdev->dev, "pid=%d, ret = %d\n",
task_pid_nr(current), ret);
+
+ drm_dbg(>drm, "<<< %s\n", __func__);
+ return ret;
+}
+
static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
mask)
{
WARN_ON(INTEL_GEN(i915) < 11);
@@ -39,6 +96,8 @@ static int intel_pxp_teardown_required_callback(struct
drm_i915_private *i915)
i915->pxp.r0ctx->global_state_attacked = true;
i915->pxp.r0ctx->flag_display_hm_surface_keys = false;
+ intel_pxp_destroy_r3ctx_list(i915);
+
mutex_unlock(>pxp.r0ctx->ctx_mutex);
return 0;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 4dec35bb834d..21a6964fc64e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -24,6 +24,21 @@ enum pxp_sm_session_req {
PXP_SM_REQ_SESSION_TERMINATE
};
+#define PXP_ACTION_SET_R3_CONTEXT 5
+
+enum pxp_sm_status {
+ PXP_SM_STATUS_SUCCESS,
+ PXP_SM_STATUS_RETRY_REQUIRED,
+ PXP_SM_STATUS_SESSION_NOT_AVAILABLE,
+ PXP_SM_STATUS_ERROR_UNKNOWN
+};
+
+struct pxp_info {
+ u32 action;
+ u32 sm_status;
+ u32 set_r3ctx;
+} __packed;
+
struct pxp_context;
struct intel_pxp {
@@ -37,6 +52,7 @@ struct intel_pxp {
struct intel_gt;
struct drm_i915_private;
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile);
void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
int i915_pxp_teardown_required_callback(struct drm_i915_private *i915);
int i915_pxp_global_terminate_complete_callback(struct drm_i915_private *i915);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_context.c
Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context (fwd)
Hello,
There is a problem on line 21 if i915 can actually be NULL.
julia
-- Forwarded message --
Date: Sat, 14 Nov 2020 19:03:47 +0800
From: kernel test robot
To: kbu...@lists.01.org
Cc: l...@intel.com, Julia Lawall
Subject: Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set
the ring3 context
CC: kbuild-...@lists.01.org
In-Reply-To: <20201114014537.25495-5-sean.z.hu...@intel.com>
References: <20201114014537.25495-5-sean.z.hu...@intel.com>
TO: Sean Z Huang
TO: Intel-gfx@lists.freedesktop.org
CC: "Huang, Sean Z"
Hi Sean,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20201113]
[also build test WARNING on v5.10-rc3]
[cannot apply to drm-intel/for-linux-next char-misc/char-misc-testing v5.10-rc3
v5.10-rc2 v5.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Sean-Z-Huang/drm-i915-pxp-Introduce-Intel-PXP-component/20201114-094926
base:92edc4aef86780a8ad01b092c6d6630bb3cb423d
:: branch date: 9 hours ago
:: commit date: 9 hours ago
config: i386-randconfig-c001-20201113 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Julia Lawall
"coccinelle warnings: (new ones prefixed by >>)"
>> drivers/gpu/drm/i915/pxp/intel_pxp.c:21:17-20: ERROR: i915 is NULL but
>> dereferenced.
vim +21 drivers/gpu/drm/i915/pxp/intel_pxp.c
a9c9ffc6ce4f8b0 Huang, Sean Z 2020-11-13 10
1d109ada10e82c3 Huang, Sean Z 2020-11-13 11 int i915_pxp_ops_ioctl(struct
drm_device *dev, void *data, struct drm_file *drmfile)
1d109ada10e82c3 Huang, Sean Z 2020-11-13 12 {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 13int ret;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 14struct pxp_info pxp_info = {0};
1d109ada10e82c3 Huang, Sean Z 2020-11-13 15struct drm_i915_pxp_ops
*pxp_ops = data;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 16struct drm_i915_private *i915 =
to_i915(dev);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 17
1d109ada10e82c3 Huang, Sean Z 2020-11-13 18drm_dbg(>drm, ">>> %s\n",
__func__);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 19
1d109ada10e82c3 Huang, Sean Z 2020-11-13 20if (!i915 || !drmfile ||
!pxp_ops || pxp_ops->pxp_info_size != sizeof(pxp_info)) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 @21drm_dbg(>drm,
"Failed to %s, invalid params\n", __func__);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 22ret = -EINVAL;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 23goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 24}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 25
1d109ada10e82c3 Huang, Sean Z 2020-11-13 26if (copy_from_user(_info,
(void __user *)pxp_ops->pxp_info_ptr, sizeof(pxp_info)) != 0) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 27ret = -EFAULT;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 28goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 29}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 30
1d109ada10e82c3 Huang, Sean Z 2020-11-13 31drm_dbg(>drm, "i915 pxp
ioctl call with action=[%d]\n", pxp_info.action);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 32
1d109ada10e82c3 Huang, Sean Z 2020-11-13 33
mutex_lock(>pxp.r0ctx->ctx_mutex);
1d109ada10e82c3 Huang, Sean Z 2020-11-13 34
1d109ada10e82c3 Huang, Sean Z 2020-11-13 35if
(i915->pxp.r0ctx->global_state_in_suspend) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 36drm_dbg(>drm,
"Return failure due to state in suspend\n");
1d109ada10e82c3 Huang, Sean Z 2020-11-13 37pxp_info.sm_status =
PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 38ret = 0;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 39goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 40}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 41
1d109ada10e82c3 Huang, Sean Z 2020-11-13 42if
(i915->pxp.r0ctx->global_state_attacked) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 43drm_dbg(>drm,
"Retry required due to state attacked\n");
1d109ada10e82c3 Huang, Sean Z 2020-11-13 44pxp_info.sm_status =
PXP_SM_STATUS_RETRY_REQUIRED;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 45ret = 0;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 46goto end;
1d109ada10e82c3 Huang, Sean Z 2020-11-13 47}
1d109ada10e82c3 Huang, Sean Z 2020-11-13 48
1d109ada10e82c3 Huang, Sean Z 2020-11-13 49switch (pxp_info.action) {
1d109ada10e82c3 Huang, Sean Z 2020-11-13 50case PXP_ACTION_SET_R3_CONTEXT:
1d109ada10e82c3 Huang, Sean Z 2020-11-13 51{
1d109ada10e82c3 Huang,
Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Hi Sean,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20201113]
[also build test WARNING on v5.10-rc3]
[cannot apply to drm-intel/for-linux-next char-misc/char-misc-testing v5.10-rc3
v5.10-rc2 v5.10-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Sean-Z-Huang/drm-i915-pxp-Introduce-Intel-PXP-component/20201114-094926
base:92edc4aef86780a8ad01b092c6d6630bb3cb423d
config: i386-randconfig-a012-20201113 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
#
https://github.com/0day-ci/linux/commit/1d109ada10e82c324682792cb0a20deef302336e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Sean-Z-Huang/drm-i915-pxp-Introduce-Intel-PXP-component/20201114-094926
git checkout 1d109ada10e82c324682792cb0a20deef302336e
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/gpu/drm/i915/pxp/intel_pxp.c: In function 'i915_pxp_ops_ioctl':
>> drivers/gpu/drm/i915/pxp/intel_pxp.c:26:32: warning: cast to pointer from
>> integer of different size [-Wint-to-pointer-cast]
26 | if (copy_from_user(_info, (void __user *)pxp_ops->pxp_info_ptr,
sizeof(pxp_info)) != 0) {
|^
drivers/gpu/drm/i915/pxp/intel_pxp.c:65:20: warning: cast to pointer from
integer of different size [-Wint-to-pointer-cast]
65 | if (copy_to_user((void __user *)pxp_ops->pxp_info_ptr, _info,
sizeof(pxp_info)) != 0)
|^
vim +26 drivers/gpu/drm/i915/pxp/intel_pxp.c
10
11 int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct
drm_file *drmfile)
12 {
13 int ret;
14 struct pxp_info pxp_info = {0};
15 struct drm_i915_pxp_ops *pxp_ops = data;
16 struct drm_i915_private *i915 = to_i915(dev);
17
18 drm_dbg(>drm, ">>> %s\n", __func__);
19
20 if (!i915 || !drmfile || !pxp_ops || pxp_ops->pxp_info_size !=
sizeof(pxp_info)) {
21 drm_dbg(>drm, "Failed to %s, invalid params\n",
__func__);
22 ret = -EINVAL;
23 goto end;
24 }
25
> 26 if (copy_from_user(_info, (void __user
*)pxp_ops->pxp_info_ptr, sizeof(pxp_info)) != 0) {
27 ret = -EFAULT;
28 goto end;
29 }
30
31 drm_dbg(>drm, "i915 pxp ioctl call with action=[%d]\n",
pxp_info.action);
32
33 mutex_lock(>pxp.r0ctx->ctx_mutex);
34
35 if (i915->pxp.r0ctx->global_state_in_suspend) {
36 drm_dbg(>drm, "Return failure due to state in
suspend\n");
37 pxp_info.sm_status =
PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
38 ret = 0;
39 goto end;
40 }
41
42 if (i915->pxp.r0ctx->global_state_attacked) {
43 drm_dbg(>drm, "Retry required due to state
attacked\n");
44 pxp_info.sm_status = PXP_SM_STATUS_RETRY_REQUIRED;
45 ret = 0;
46 goto end;
47 }
48
49 switch (pxp_info.action) {
50 case PXP_ACTION_SET_R3_CONTEXT:
51 {
52 ret = intel_pxp_set_r3ctx(i915, pxp_info.set_r3ctx);
53 break;
54 }
55 default:
56 drm_dbg(>drm, "Failed to %s due to bad params\n",
__func__);
57 ret = -EINVAL;
58 goto end;
59 }
60
61 end:
62 mutex_unlock(>pxp.r0ctx->ctx_mutex);
63
64 if (ret == 0)
65 if (copy_to_user((void __user *)pxp_ops->pxp_info_ptr,
_info, sizeof(pxp_info)) != 0)
66 ret = -EFAULT;
67
68 if (ret)
69 dev_err(>pdev->dev, "pid=%d, ret = %d\n",
task_pid_nr(current), ret);
70
71 drm_dbg(>drm, "<<< %s\n", __func__);
72 return ret;
73 }
74
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
From: "Huang, Sean Z"
Enable one ioctl action to allow ring3 driver to set its ring3
context, so ring0 PXP can track the context id through this ring3
context list.
Signed-off-by: Huang, Sean Z
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 66
drivers/gpu/drm/i915/pxp/intel_pxp.h | 16 +
drivers/gpu/drm/i915/pxp/intel_pxp_context.c | 34 ++
drivers/gpu/drm/i915/pxp/intel_pxp_context.h | 3 +
include/uapi/drm/i915_drm.h | 2 +
6 files changed, 122 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c8b9c42fcbd6..43ea85b5f14b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1761,6 +1761,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl,
DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl,
DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(I915_PXP_OPS, i915_pxp_ops_ioctl, DRM_RENDER_ALLOW),
};
static const struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 3a24c2b13b14..a83fa7cd749f 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -8,6 +8,70 @@
#include "intel_pxp_context.h"
#include "intel_pxp_sm.h"
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile)
+{
+ int ret;
+ struct pxp_info pxp_info = {0};
+ struct drm_i915_pxp_ops *pxp_ops = data;
+ struct drm_i915_private *i915 = to_i915(dev);
+
+ drm_dbg(>drm, ">>> %s\n", __func__);
+
+ if (!i915 || !drmfile || !pxp_ops || pxp_ops->pxp_info_size !=
sizeof(pxp_info)) {
+ drm_dbg(>drm, "Failed to %s, invalid params\n", __func__);
+ ret = -EINVAL;
+ goto end;
+ }
+
+ if (copy_from_user(_info, (void __user *)pxp_ops->pxp_info_ptr,
sizeof(pxp_info)) != 0) {
+ ret = -EFAULT;
+ goto end;
+ }
+
+ drm_dbg(>drm, "i915 pxp ioctl call with action=[%d]\n",
pxp_info.action);
+
+ mutex_lock(>pxp.r0ctx->ctx_mutex);
+
+ if (i915->pxp.r0ctx->global_state_in_suspend) {
+ drm_dbg(>drm, "Return failure due to state in suspend\n");
+ pxp_info.sm_status = PXP_SM_STATUS_SESSION_NOT_AVAILABLE;
+ ret = 0;
+ goto end;
+ }
+
+ if (i915->pxp.r0ctx->global_state_attacked) {
+ drm_dbg(>drm, "Retry required due to state attacked\n");
+ pxp_info.sm_status = PXP_SM_STATUS_RETRY_REQUIRED;
+ ret = 0;
+ goto end;
+ }
+
+ switch (pxp_info.action) {
+ case PXP_ACTION_SET_R3_CONTEXT:
+ {
+ ret = intel_pxp_set_r3ctx(i915, pxp_info.set_r3ctx);
+ break;
+ }
+ default:
+ drm_dbg(>drm, "Failed to %s due to bad params\n",
__func__);
+ ret = -EINVAL;
+ goto end;
+ }
+
+end:
+ mutex_unlock(>pxp.r0ctx->ctx_mutex);
+
+ if (ret == 0)
+ if (copy_to_user((void __user *)pxp_ops->pxp_info_ptr,
_info, sizeof(pxp_info)) != 0)
+ ret = -EFAULT;
+
+ if (ret)
+ dev_err(>pdev->dev, "pid=%d, ret = %d\n",
task_pid_nr(current), ret);
+
+ drm_dbg(>drm, "<<< %s\n", __func__);
+ return ret;
+}
+
static void intel_pxp_write_irq_mask_reg(struct drm_i915_private *i915, u32
mask)
{
WARN_ON(INTEL_GEN(i915) < 11);
@@ -39,6 +103,8 @@ static int intel_pxp_teardown_required_callback(struct
drm_i915_private *i915)
i915->pxp.r0ctx->global_state_attacked = true;
i915->pxp.r0ctx->flag_display_hm_surface_keys = false;
+ intel_pxp_destroy_r3ctx_list(i915);
+
mutex_unlock(>pxp.r0ctx->ctx_mutex);
return 0;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h
b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 4dec35bb834d..21a6964fc64e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -24,6 +24,21 @@ enum pxp_sm_session_req {
PXP_SM_REQ_SESSION_TERMINATE
};
+#define PXP_ACTION_SET_R3_CONTEXT 5
+
+enum pxp_sm_status {
+ PXP_SM_STATUS_SUCCESS,
+ PXP_SM_STATUS_RETRY_REQUIRED,
+ PXP_SM_STATUS_SESSION_NOT_AVAILABLE,
+ PXP_SM_STATUS_ERROR_UNKNOWN
+};
+
+struct pxp_info {
+ u32 action;
+ u32 sm_status;
+ u32 set_r3ctx;
+} __packed;
+
struct pxp_context;
struct intel_pxp {
@@ -37,6 +52,7 @@ struct intel_pxp {
struct intel_gt;
struct drm_i915_private;
+int i915_pxp_ops_ioctl(struct drm_device *dev, void *data, struct drm_file
*drmfile);
void intel_pxp_irq_handler(struct intel_gt *gt, u16 iir);
