Date:   Fri, 27 Jan 2023 03:21:50 +0000
From:   Matthew Brost <matthew.brost@intel.com>
To:     Danilo Krummrich <dakr@redhat.com>
CC:     <daniel@ffwll.ch>, <airlied@redhat.com>,
        <christian.koenig@amd.com>, <bskeggs@redhat.com>,
        <jason@jlekstrand.net>, <tzimmermann@suse.de>,
        <mripard@kernel.org>, <corbet@lwn.net>,
        <nouveau@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        <dri-devel@lists.freedesktop.org>, <linux-doc@vger.kernel.org>
Subject: Re: [PATCH drm-next 05/14] drm/nouveau: new VM_BIND uapi interfaces
Message-ID: <Y9NDTrGhSXomICEE@DUT025-TGLU.fm.intel.com>
References: <20230118061256.2689-1-dakr@redhat.com>
 <20230118061256.2689-6-dakr@redhat.com>
 <Y9MjSeMcsd18r9vM@DUT025-TGLU.fm.intel.com>
 <0bf3fec7-5aac-d3cb-8953-a332f84bdf8a@redhat.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <0bf3fec7-5aac-d3cb-8953-a332f84bdf8a@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?L9YFl4SiQFXQmVT1LXAwF3cX8tERunUkv9y4djRQ6O55sTRMrqncK24ZHkXY?=
 =?us-ascii?Q?n9kKIDEWR6ypOa1JglC2V6m3llo7hh2HLFYY7GCfGpagITihDrY4KVf2D6CC?=
 =?us-ascii?Q?28LbnRP/bNLmLXH4JLR33fnZ9y+fahbMIVDeF8iQM9vQSTR8N6lLMNIftX7R?=
 =?us-ascii?Q?lD+K5vYsUsvPsP9C1eOZMa2wSWvlqcPWz6181BPwsYPTobFyG6UL+jXVW9WU?=
 =?us-ascii?Q?lpvuut82Ao/vm3DhTAY4D+ClNJU1wrJUFXvDmTQ2VzN5C9W3iHdkshooEd7G?=
 =?us-ascii?Q?nfdrlETtbb2/ttjQs+Myy2Liq6cmvVqiqYE5Kc+t65X5p0P/pUyc60dImQqP?=
 =?us-ascii?Q?lv+Bru37yctrmBiScc8M5icR1/Esc3qCmV2szmIJR+z9vil3APnfz5uwXSmk?=
 =?us-ascii?Q?xXGmtzub5rl5461M4JFy4g/8ITIJsSMFoza/3OehWErwYfh953hAysav4Tba?=
 =?us-ascii?Q?3zs1kClCWspHwHVHlcHxbYgPqNrAKna5FCsOUW4EnqRZYKrwTuHNfroNUO43?=
 =?us-ascii?Q?kDThI/Pnk2HTB/6Nc2qR2GgrFEyAMnNTsM00bBv+iYv2caN03uwxDz/uLX5j?=
 =?us-ascii?Q?wtcI7q8anRIu7O8OADaNNwhR2p3xi32CYMGamZKsYDCMRENmrCbNBV391w7l?=
 =?us-ascii?Q?GUPThh6uOARwBAP0klSWq3VVHm7ZB8evhzK4lHjVM0c62my7jlDTB39DNpWf?=
 =?us-ascii?Q?1hjSSxoNJ+ukYp8MlK3BtV3hFS+N6xc1Pw1sy1rONn7DvlOUSdiAky0oaaXu?=
 =?us-ascii?Q?vxVypj09WY+xfq+oXTGHVoNrKckXEJwz3177BOfCV/MvZMRDk6sAYftpR7oB?=
 =?us-ascii?Q?1lMkBWXIRQY91iIkn/vN2jXJYrhJrU9k+thFOG8iIULJrg/nh7BXeU/dLPWv?=
 =?us-ascii?Q?j8ZG1p6RF10ULalUrIhNclDDhBOy/wBolEr324UU+1wx51kdxQ+f03ztf4Ah?=
 =?us-ascii?Q?x/E85uI8iPJcEQgU3s6xiHU1lzwclzXjLHk8OZmP5y4mrcRUY7xs8fU8WBHj?=
 =?us-ascii?Q?zcBhrG71UKcNGcFSzVQdsZZC1CPy19I6cRFfP3Lb56b9M736xcRHSrBw/k+f?=
 =?us-ascii?Q?QPFa2RmhSaLxoj8BMzPEVDa0AnV9Hj3MqWfO4l0rph0ysPJUrFBMe1gwPzLp?=
 =?us-ascii?Q?JdyVs5zwDM7Cl5T6XBltRplRsXkVX3EJxp4eH0h+mBCsMVldGdRC2TTf0XHv?=
 =?us-ascii?Q?Fyf+OnSa82NqZOsPSXch3Nou3eEsHtRJP8pdFDu9ia9jeO0V6nmxqS8xtLpu?=
 =?us-ascii?Q?PgiBZPY0ls+NxE29hVdDTAlnjBp+04W/KLQrdPFGCBvUGTok5LQ6XuAlYoGS?=
 =?us-ascii?Q?REDKZ72tJhRcSIu4Pt82N0lg1XpPZWGkNPkMapngYFquqB1izLhotUzKbbF5?=
 =?us-ascii?Q?xYtE9gBAa28sTlLxxZ5XSbzISbUyvqsAgcK44xwqVi2rFq3i//XvUlHMUZDq?=
 =?us-ascii?Q?GaDiSNPpqU+oBy9ktcUu0//LCimljgz/dvUnAJ/jKydUCi/lVNm6bxhZAnBQ?=
 =?us-ascii?Q?1i6222/t7Ral9/CEX0Ppm4XEjrCJ65LLPi24uFX0MU/5Fjs4rQ7bpEIj3N9n?=
 =?us-ascii?Q?csM3vV/WKPhXUO0FZ31lsccYWAkEOLCu+QaBu9leMY6xFtBe6J8lgucSi6MH?=
 =?us-ascii?Q?Aw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a264c72-6893-4546-40a5-08db0015ab74
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB6522.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jan 2023 03:22:07.5611
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: t/bKgOG4SP54W80S+XH1VZQDCv/L5/Rf0igNEI9GWbCFQlkrak9pwdFz7jJ+1XTYz6arlbg1axu+yB9DgpHW2g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6483
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Jan 27, 2023 at 02:43:30AM +0100, Danilo Krummrich wrote:
> 
> 
> On 1/27/23 02:05, Matthew Brost wrote:
> > On Wed, Jan 18, 2023 at 07:12:47AM +0100, Danilo Krummrich wrote:
> > > This commit provides the interfaces for the new UAPI motivated by the
> > > Vulkan API. It allows user mode drivers (UMDs) to:
> > > 
> > > 1) Initialize a GPU virtual address (VA) space via the new
> > >     DRM_IOCTL_NOUVEAU_VM_INIT ioctl. UMDs can provide a kernel reserved
> > >     VA area.
> > > 
> > > 2) Bind and unbind GPU VA space mappings via the new
> > >     DRM_IOCTL_NOUVEAU_VM_BIND ioctl.
> > > 
> > > 3) Execute push buffers with the new DRM_IOCTL_NOUVEAU_EXEC ioctl.
> > > 
> > > Both, DRM_IOCTL_NOUVEAU_VM_BIND and DRM_IOCTL_NOUVEAU_EXEC support
> > > asynchronous processing with DRM syncobjs as synchronization mechanism.
> > > 
> > > The default DRM_IOCTL_NOUVEAU_VM_BIND is synchronous processing,
> > > DRM_IOCTL_NOUVEAU_EXEC supports asynchronous processing only.
> > > 
> > > Co-authored-by: Dave Airlie <airlied@redhat.com>
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >   Documentation/gpu/driver-uapi.rst |   8 ++
> > >   include/uapi/drm/nouveau_drm.h    | 216 ++++++++++++++++++++++++++++++
> > >   2 files changed, 224 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/driver-uapi.rst b/Documentation/gpu/driver-uapi.rst
> > > index 4411e6919a3d..9c7ca6e33a68 100644
> > > --- a/Documentation/gpu/driver-uapi.rst
> > > +++ b/Documentation/gpu/driver-uapi.rst
> > > @@ -6,3 +6,11 @@ drm/i915 uAPI
> > >   =============
> > >   .. kernel-doc:: include/uapi/drm/i915_drm.h
> > > +
> > > +drm/nouveau uAPI
> > > +================
> > > +
> > > +VM_BIND / EXEC uAPI
> > > +-------------------
> > > +
> > > +.. kernel-doc:: include/uapi/drm/nouveau_drm.h
> > > diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h
> > > index 853a327433d3..f6e7d40201d4 100644
> > > --- a/include/uapi/drm/nouveau_drm.h
> > > +++ b/include/uapi/drm/nouveau_drm.h
> > > @@ -126,6 +126,216 @@ struct drm_nouveau_gem_cpu_fini {
> > >   	__u32 handle;
> > >   };
> > > +/**
> > > + * struct drm_nouveau_sync - sync object
> > > + *
> > > + * This structure serves as synchronization mechanism for (potentially)
> > > + * asynchronous operations such as EXEC or VM_BIND.
> > > + */
> > > +struct drm_nouveau_sync {
> > > +	/**
> > > +	 * @flags: the flags for a sync object
> > > +	 *
> > > +	 * The first 8 bits are used to determine the type of the sync object.
> > > +	 */
> > > +	__u32 flags;
> > > +#define DRM_NOUVEAU_SYNC_SYNCOBJ 0x0
> > > +#define DRM_NOUVEAU_SYNC_TIMELINE_SYNCOBJ 0x1
> > > +#define DRM_NOUVEAU_SYNC_TYPE_MASK 0xf
> > > +	/**
> > > +	 * @handle: the handle of the sync object
> > > +	 */
> > > +	__u32 handle;
> > > +	/**
> > > +	 * @timeline_value:
> > > +	 *
> > > +	 * The timeline point of the sync object in case the syncobj is of
> > > +	 * type DRM_NOUVEAU_SYNC_TIMELINE_SYNCOBJ.
> > > +	 */
> > > +	__u64 timeline_value;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_nouveau_vm_init - GPU VA space init structure
> > > + *
> > > + * Used to initialize the GPU's VA space for a user client, telling the kernel
> > > + * which portion of the VA space is managed by the UMD and kernel respectively.
> > > + */
> > > +struct drm_nouveau_vm_init {
> > > +	/**
> > > +	 * @unmanaged_addr: start address of the kernel managed VA space region
> > > +	 */
> > > +	__u64 unmanaged_addr;
> > > +	/**
> > > +	 * @unmanaged_size: size of the kernel managed VA space region in bytes
> > > +	 */
> > > +	__u64 unmanaged_size;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_nouveau_vm_bind_op - VM_BIND operation
> > > + *
> > > + * This structure represents a single VM_BIND operation. UMDs should pass
> > > + * an array of this structure via struct drm_nouveau_vm_bind's &op_ptr field.
> > > + */
> > > +struct drm_nouveau_vm_bind_op {
> > > +	/**
> > > +	 * @op: the operation type
> > > +	 */
> > > +	__u32 op;
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_OP_ALLOC:
> > > + *
> > > + * The alloc operation is used to reserve a VA space region within the GPU's VA
> > > + * space. Optionally, the &DRM_NOUVEAU_VM_BIND_SPARSE flag can be passed to
> > > + * instruct the kernel to create sparse mappings for the given region.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_OP_ALLOC 0x0
> > 
> > Do you really need this operation? We have no concept of this in Xe,
> > e.g. we can create a VM and the entire address space is managed exactly
> > the same.
> > 
> > If this can be removed then the entire concept of regions in the GPUVA
> > can be removed too (drop struct drm_gpuva_region). I say this because
> > in Xe as I'm porting over to GPUVA the first thing I'm doing after
> > drm_gpuva_manager_init is calling drm_gpuva_region_insert on the entire
> > address space.
> 
> Also, since you've been starting to use the code, this [1] is the branch I'm
> pushing my fixes for a v2 to. It already contains the changes for the GPUVA
> manager except for switching away from drm_mm.
> 
> [1] https://gitlab.freedesktop.org/nouvelles/kernel/-/tree/new-uapi-drm-next-fixes
> 

I will take a look at this branch. I believe you are on our Xe gitlab
project (working on getting this public) so you can comment on any MR I
post there, I expect to have something posted early next week to port Xe
to the gpuva.

Also I assume you are dri-devel IRC, what is your handle? Mine is
mbrost. It might be useful to chat in real time.

Matt

> > To me this seems kinda useless but maybe I'm missing why
> > you need this for Nouveau.
> > 
> > Matt
> > 
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_OP_FREE: Free a reserved VA space region.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_OP_FREE 0x1
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_OP_MAP:
> > > + *
> > > + * Map a GEM object to the GPU's VA space. The mapping must be fully enclosed by
> > > + * a previously allocated VA space region. If the region is sparse, existing
> > > + * sparse mappings are overwritten.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_OP_MAP 0x2
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_OP_UNMAP:
> > > + *
> > > + * Unmap an existing mapping in the GPU's VA space. If the region the mapping
> > > + * is located in is a sparse region, new sparse mappings are created where the
> > > + * unmapped (memory backed) mapping was mapped previously.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x3
> > > +	/**
> > > +	 * @flags: the flags for a &drm_nouveau_vm_bind_op
> > > +	 */
> > > +	__u32 flags;
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_SPARSE:
> > > + *
> > > + * Indicates that an allocated VA space region should be sparse.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8)
> > > +	/**
> > > +	 * @handle: the handle of the DRM GEM object to map
> > > +	 */
> > > +	__u32 handle;
> > > +	/**
> > > +	 * @addr:
> > > +	 *
> > > +	 * the address the VA space region or (memory backed) mapping should be mapped to
> > > +	 */
> > > +	__u64 addr;
> > > +	/**
> > > +	 * @bo_offset: the offset within the BO backing the mapping
> > > +	 */
> > > +	__u64 bo_offset;
> > > +	/**
> > > +	 * @range: the size of the requested mapping in bytes
> > > +	 */
> > > +	__u64 range;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_nouveau_vm_bind - structure for DRM_IOCTL_NOUVEAU_VM_BIND
> > > + */
> > > +struct drm_nouveau_vm_bind {
> > > +	/**
> > > +	 * @op_count: the number of &drm_nouveau_vm_bind_op
> > > +	 */
> > > +	__u32 op_count;
> > > +	/**
> > > +	 * @flags: the flags for a &drm_nouveau_vm_bind ioctl
> > > +	 */
> > > +	__u32 flags;
> > > +/**
> > > + * @DRM_NOUVEAU_VM_BIND_RUN_ASYNC:
> > > + *
> > > + * Indicates that the given VM_BIND operation should be executed asynchronously
> > > + * by the kernel.
> > > + *
> > > + * If this flag is not supplied the kernel executes the associated operations
> > > + * synchronously and doesn't accept any &drm_nouveau_sync objects.
> > > + */
> > > +#define DRM_NOUVEAU_VM_BIND_RUN_ASYNC 0x1
> > > +	/**
> > > +	 * @wait_count: the number of wait &drm_nouveau_syncs
> > > +	 */
> > > +	__u32 wait_count;
> > > +	/**
> > > +	 * @sig_count: the number of &drm_nouveau_syncs to signal when finished
> > > +	 */
> > > +	__u32 sig_count;
> > > +	/**
> > > +	 * @wait_ptr: pointer to &drm_nouveau_syncs to wait for
> > > +	 */
> > > +	__u64 wait_ptr;
> > > +	/**
> > > +	 * @sig_ptr: pointer to &drm_nouveau_syncs to signal when finished
> > > +	 */
> > > +	__u64 sig_ptr;
> > > +	/**
> > > +	 * @op_ptr: pointer to the &drm_nouveau_vm_bind_ops to execute
> > > +	 */
> > > +	__u64 op_ptr;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_nouveau_exec_push - EXEC push operation
> > > + *
> > > + * This structure represents a single EXEC push operation. UMDs should pass an
> > > + * array of this structure via struct drm_nouveau_exec's &push_ptr field.
> > > + */
> > > +struct drm_nouveau_exec_push {
> > > +	/**
> > > +	 * @va: the virtual address of the push buffer mapping
> > > +	 */
> > > +	__u64 va;
> > > +	/**
> > > +	 * @va_len: the length of the push buffer mapping
> > > +	 */
> > > +	__u64 va_len;
> > > +};
> > > +
> > > +/**
> > > + * struct drm_nouveau_exec - structure for DRM_IOCTL_NOUVEAU_EXEC
> > > + */
> > > +struct drm_nouveau_exec {
> > > +	/**
> > > +	 * @channel: the channel to execute the push buffer in
> > > +	 */
> > > +	__u32 channel;
> > > +	/**
> > > +	 * @push_count: the number of &drm_nouveau_exec_push ops
> > > +	 */
> > > +	__u32 push_count;
> > > +	/**
> > > +	 * @wait_count: the number of wait &drm_nouveau_syncs
> > > +	 */
> > > +	__u32 wait_count;
> > > +	/**
> > > +	 * @sig_count: the number of &drm_nouveau_syncs to signal when finished
> > > +	 */
> > > +	__u32 sig_count;
> > > +	/**
> > > +	 * @wait_ptr: pointer to &drm_nouveau_syncs to wait for
> > > +	 */
> > > +	__u64 wait_ptr;
> > > +	/**
> > > +	 * @sig_ptr: pointer to &drm_nouveau_syncs to signal when finished
> > > +	 */
> > > +	__u64 sig_ptr;
> > > +	/**
> > > +	 * @push_ptr: pointer to &drm_nouveau_exec_push ops
> > > +	 */
> > > +	__u64 push_ptr;
> > > +};
> > > +
> > >   #define DRM_NOUVEAU_GETPARAM           0x00 /* deprecated */
> > >   #define DRM_NOUVEAU_SETPARAM           0x01 /* deprecated */
> > >   #define DRM_NOUVEAU_CHANNEL_ALLOC      0x02 /* deprecated */
> > > @@ -136,6 +346,9 @@ struct drm_nouveau_gem_cpu_fini {
> > >   #define DRM_NOUVEAU_NVIF               0x07
> > >   #define DRM_NOUVEAU_SVM_INIT           0x08
> > >   #define DRM_NOUVEAU_SVM_BIND           0x09
> > > +#define DRM_NOUVEAU_VM_INIT            0x10
> > > +#define DRM_NOUVEAU_VM_BIND            0x11
> > > +#define DRM_NOUVEAU_EXEC               0x12
> > >   #define DRM_NOUVEAU_GEM_NEW            0x40
> > >   #define DRM_NOUVEAU_GEM_PUSHBUF        0x41
> > >   #define DRM_NOUVEAU_GEM_CPU_PREP       0x42
> > > @@ -197,6 +410,9 @@ struct drm_nouveau_svm_bind {
> > >   #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI       DRM_IOW (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct drm_nouveau_gem_cpu_fini)
> > >   #define DRM_IOCTL_NOUVEAU_GEM_INFO           DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct drm_nouveau_gem_info)
> > > +#define DRM_IOCTL_NOUVEAU_VM_INIT            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_VM_INIT, struct drm_nouveau_vm_init)
> > > +#define DRM_IOCTL_NOUVEAU_VM_BIND            DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_VM_BIND, struct drm_nouveau_vm_bind)
> > > +#define DRM_IOCTL_NOUVEAU_EXEC               DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_EXEC, struct drm_nouveau_exec)
> > >   #if defined(__cplusplus)
> > >   }
> > >   #endif
> > > -- 
> > > 2.39.0
> > > 
> > 
>