Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: "Huang, Sean Z" <sean.z.huang@intel.com>,
	Intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 05/27] drm/i915/pxp: Enable ioctl action to set the ring3 context
Date: Mon, 16 Nov 2020 12:22:23 +0200	[thread overview]
Message-ID: <160552214303.10586.7473931663804369779@jlahtine-mobl.ger.corp.intel.com> (raw)
In-Reply-To: <20201115210815.5272-5-sean.z.huang@intel.com>

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 <sean.z.huang@intel.com>

<SNIP>

> +++ 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_DESTROY                0x3b
> +#define DRM_I915_PXP_OPS               0x3c
>  /* Must be kept compact -- no holes */
>  
>  #define DRM_IOCTL_I915_INIT            DRM_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