On Wed, Jan 16, 2019, 7:55 AM Christian König < ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote: > Well if you ask me we should have the following interface for > negotiating memory management with the kernel: > > 1. We have per process BOs which can't be shared between processes. > > Those are always valid and don't need to be mentioned in any BO list > whatsoever. > > If we knew that a per process BO is currently not in use we can > optionally tell that to the kernel to make memory management more > efficient. > > In other words instead of a list of stuff which is used we send down to > the kernel a list of stuff which is not used any more and that only when > we know that it is necessary, e.g. when a game or application overcommits. > Radeonsi doesn't use this because this approach caused performance degradation and also drops BO priorities. Marek > 2. We have shared BOs which are used by more than one process. > > Those are rare and should be added to the per CS list of BOs in use. > > > The whole BO list interface Marek tries to optimize here should be > deprecated and not used any more. > > Regards, > Christian. > > Am 16.01.19 um 13:46 schrieb Bas Nieuwenhuizen: > > So random questions: > > > > 1) In this discussion it was mentioned that some Vulkan drivers still > > use the bo_list interface. I think that implies radv as I think we're > > still using bo_list. Is there any other API we should be using? (Also, > > with VK_EXT_descriptor_indexing I suspect we'll be moving more towards > > a global bo list instead of a cmd buffer one, as we cannot know all > > the BOs referenced anymore, but not sure what end state here will be). > > > > 2) The other alternative mentioned was adding the buffers directly > > into the submit ioctl. Is this the desired end state (though as above > > I'm not sure how that works for vulkan)? If yes, what is the timeline > > for this that we need something in the interim? > > > > 3) Did we measure any performance benefit? > > > > In general I'd like to to ack the raw bo list creation function as > > this interface seems easier to use. The two arrays thing has always > > been kind of a pain when we want to use e.g. builtin sort functions to > > make sure we have no duplicate BOs, but have some comments below. > > > > On Mon, Jan 7, 2019 at 8:31 PM Marek Olšák wrote: > >> From: Marek Olšák > >> > >> --- > >> amdgpu/amdgpu-symbol-check | 3 ++ > >> amdgpu/amdgpu.h | 56 +++++++++++++++++++++++++++++++++++++- > >> amdgpu/amdgpu_bo.c | 36 ++++++++++++++++++++++++ > >> amdgpu/amdgpu_cs.c | 25 +++++++++++++++++ > >> 4 files changed, 119 insertions(+), 1 deletion(-) > >> > >> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check > >> index 6f5e0f95..96a44b40 100755 > >> --- a/amdgpu/amdgpu-symbol-check > >> +++ b/amdgpu/amdgpu-symbol-check > >> @@ -12,20 +12,22 @@ _edata > >> _end > >> _fini > >> _init > >> amdgpu_bo_alloc > >> amdgpu_bo_cpu_map > >> amdgpu_bo_cpu_unmap > >> amdgpu_bo_export > >> amdgpu_bo_free > >> amdgpu_bo_import > >> amdgpu_bo_inc_ref > >> +amdgpu_bo_list_create_raw > >> +amdgpu_bo_list_destroy_raw > >> amdgpu_bo_list_create > >> amdgpu_bo_list_destroy > >> amdgpu_bo_list_update > >> amdgpu_bo_query_info > >> amdgpu_bo_set_metadata > >> amdgpu_bo_va_op > >> amdgpu_bo_va_op_raw > >> amdgpu_bo_wait_for_idle > >> amdgpu_create_bo_from_user_mem > >> amdgpu_cs_chunk_fence_info_to_data > >> @@ -40,20 +42,21 @@ amdgpu_cs_destroy_semaphore > >> amdgpu_cs_destroy_syncobj > >> amdgpu_cs_export_syncobj > >> amdgpu_cs_fence_to_handle > >> amdgpu_cs_import_syncobj > >> amdgpu_cs_query_fence_status > >> amdgpu_cs_query_reset_state > >> amdgpu_query_sw_info > >> amdgpu_cs_signal_semaphore > >> amdgpu_cs_submit > >> amdgpu_cs_submit_raw > >> +amdgpu_cs_submit_raw2 > >> amdgpu_cs_syncobj_export_sync_file > >> amdgpu_cs_syncobj_import_sync_file > >> amdgpu_cs_syncobj_reset > >> amdgpu_cs_syncobj_signal > >> amdgpu_cs_syncobj_wait > >> amdgpu_cs_wait_fences > >> amdgpu_cs_wait_semaphore > >> amdgpu_device_deinitialize > >> amdgpu_device_initialize > >> amdgpu_find_bo_by_cpu_mapping > >> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h > >> index dc51659a..5b800033 100644 > >> --- a/amdgpu/amdgpu.h > >> +++ b/amdgpu/amdgpu.h > >> @@ -35,20 +35,21 @@ > >> #define _AMDGPU_H_ > >> > >> #include > >> #include > >> > >> #ifdef __cplusplus > >> extern "C" { > >> #endif > >> > >> struct drm_amdgpu_info_hw_ip; > >> +struct drm_amdgpu_bo_list_entry; > >> > >> > /*--------------------------------------------------------------------------*/ > >> /* --------------------------- Defines > ------------------------------------ */ > >> > /*--------------------------------------------------------------------------*/ > >> > >> /** > >> * Define max. number of Command Buffers (IB) which could be sent to > the single > >> * hardware IP to accommodate CE/DE requirements > >> * > >> * \sa amdgpu_cs_ib_info > >> @@ -767,34 +768,65 @@ int amdgpu_bo_cpu_unmap(amdgpu_bo_handle > buf_handle); > >> * and no GPU access is scheduled. > >> * 1 GPU access is in fly or scheduled > >> * > >> * \return 0 - on success > >> * <0 - Negative POSIX Error code > >> */ > >> int amdgpu_bo_wait_for_idle(amdgpu_bo_handle buf_handle, > >> uint64_t timeout_ns, > >> bool *buffer_busy); > >> > >> +/** > >> + * Creates a BO list handle for command submission. > >> + * > >> + * \param dev - \c [in] Device handle. > >> + * See #amdgpu_device_initialize() > >> + * \param number_of_buffers - \c [in] Number of BOs in the list > >> + * \param buffers - \c [in] List of BO handles > >> + * \param result - \c [out] Created BO list handle > >> + * > >> + * \return 0 on success\n > >> + * <0 - Negative POSIX Error code > >> + * > >> + * \sa amdgpu_bo_list_destroy_raw() > >> +*/ > >> +int amdgpu_bo_list_create_raw(amdgpu_device_handle dev, > >> + uint32_t number_of_buffers, > >> + struct drm_amdgpu_bo_list_entry *buffers, > >> + uint32_t *result); > > So AFAIU drm_amdgpu_bo_list_entry takes a raw bo handle while we > > never get a raw bo handle from libdrm_amdgpu. How are we supposed to > > fill it in? > > > > What do we win by having the raw handle for the bo_list? If we would > > not return the raw handle we would not need the submit_raw2. > > > >> + > >> +/** > >> + * Destroys a BO list handle. > >> + * > >> + * \param bo_list - \c [in] BO list handle. > >> + * > >> + * \return 0 on success\n > >> + * <0 - Negative POSIX Error code > >> + * > >> + * \sa amdgpu_bo_list_create_raw(), amdgpu_cs_submit_raw2() > >> +*/ > >> +int amdgpu_bo_list_destroy_raw(amdgpu_device_handle dev, uint32_t > bo_list); > >> + > >> /** > >> * Creates a BO list handle for command submission. > >> * > >> * \param dev - \c [in] Device handle. > >> * See #amdgpu_device_initialize() > >> * \param number_of_resources - \c [in] Number of BOs in the > list > >> * \param resources - \c [in] List of BO handles > >> * \param resource_prios - \c [in] Optional priority for each > handle > >> * \param result - \c [out] Created BO list handle > >> * > >> * \return 0 on success\n > >> * <0 - Negative POSIX Error code > >> * > >> - * \sa amdgpu_bo_list_destroy() > >> + * \sa amdgpu_bo_list_destroy(), amdgpu_cs_submit_raw2() > >> */ > >> int amdgpu_bo_list_create(amdgpu_device_handle dev, > >> uint32_t number_of_resources, > >> amdgpu_bo_handle *resources, > >> uint8_t *resource_prios, > >> amdgpu_bo_list_handle *result); > >> > >> /** > >> * Destroys a BO list handle. > >> * > >> @@ -1580,20 +1612,42 @@ struct drm_amdgpu_cs_chunk; > >> struct drm_amdgpu_cs_chunk_dep; > >> struct drm_amdgpu_cs_chunk_data; > >> > >> int amdgpu_cs_submit_raw(amdgpu_device_handle dev, > >> amdgpu_context_handle context, > >> amdgpu_bo_list_handle bo_list_handle, > >> int num_chunks, > >> struct drm_amdgpu_cs_chunk *chunks, > >> uint64_t *seq_no); > >> > >> +/** > >> + * Submit raw command submission to the kernel with a raw BO list > handle. > >> + * > >> + * \param dev - \c [in] device handle > >> + * \param context - \c [in] context handle for context id > >> + * \param bo_list_handle - \c [in] raw bo list handle (0 for none) > >> + * \param num_chunks - \c [in] number of CS chunks to submit > >> + * \param chunks - \c [in] array of CS chunks > >> + * \param seq_no - \c [out] output sequence number for > submission. > >> + * > >> + * \return 0 on success\n > >> + * <0 - Negative POSIX Error code > >> + * > >> + * \sa amdgpu_bo_list_create_raw(), amdgpu_bo_list_destroy_raw() > >> + */ > >> +int amdgpu_cs_submit_raw2(amdgpu_device_handle dev, > >> + amdgpu_context_handle context, > >> + uint32_t bo_list_handle, > >> + int num_chunks, > >> + struct drm_amdgpu_cs_chunk *chunks, > >> + uint64_t *seq_no); > >> + > >> void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence *fence, > >> struct drm_amdgpu_cs_chunk_dep *dep); > >> void amdgpu_cs_chunk_fence_info_to_data(struct amdgpu_cs_fence_info > *fence_info, > >> struct > drm_amdgpu_cs_chunk_data *data); > >> > >> /** > >> * Reserve VMID > >> * \param context - \c [in] GPU Context > >> * \param flags - \c [in] TBD > >> * > >> diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c > >> index c0f42e81..21bc73aa 100644 > >> --- a/amdgpu/amdgpu_bo.c > >> +++ b/amdgpu/amdgpu_bo.c > >> @@ -611,20 +611,56 @@ drm_public int > amdgpu_create_bo_from_user_mem(amdgpu_device_handle dev, > >> pthread_mutex_lock(&dev->bo_table_mutex); > >> r = handle_table_insert(&dev->bo_handles, > (*buf_handle)->handle, > >> *buf_handle); > >> pthread_mutex_unlock(&dev->bo_table_mutex); > >> if (r) > >> amdgpu_bo_free(*buf_handle); > >> out: > >> return r; > >> } > >> > >> +drm_public int amdgpu_bo_list_create_raw(amdgpu_device_handle dev, > >> + uint32_t number_of_buffers, > >> + struct > drm_amdgpu_bo_list_entry *buffers, > >> + uint32_t *result) > >> +{ > >> + union drm_amdgpu_bo_list args; > >> + int r; > >> + > >> + memset(&args, 0, sizeof(args)); > >> + args.in.operation = AMDGPU_BO_LIST_OP_CREATE; > >> + args.in.bo_number = number_of_buffers; > >> + args.in.bo_info_size = sizeof(struct drm_amdgpu_bo_list_entry); > >> + args.in.bo_info_ptr = (uint64_t)(uintptr_t)buffers; > >> + > >> + r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_BO_LIST, > >> + &args, sizeof(args)); > >> + if (r) > >> + return r; > >> + > >> + *result = args.out.list_handle; > >> + return 0; > >> +} > >> + > >> +drm_public int amdgpu_bo_list_destroy_raw(amdgpu_device_handle dev, > >> + uint32_t bo_list) > >> +{ > >> + union drm_amdgpu_bo_list args; > >> + > >> + memset(&args, 0, sizeof(args)); > >> + args.in.operation = AMDGPU_BO_LIST_OP_DESTROY; > >> + args.in.list_handle = bo_list; > >> + > >> + return drmCommandWriteRead(dev->fd, DRM_AMDGPU_BO_LIST, > >> + &args, sizeof(args)); > >> +} > >> + > >> drm_public int amdgpu_bo_list_create(amdgpu_device_handle dev, > >> uint32_t number_of_resources, > >> amdgpu_bo_handle *resources, > >> uint8_t *resource_prios, > >> amdgpu_bo_list_handle *result) > >> { > >> struct drm_amdgpu_bo_list_entry *list; > >> union drm_amdgpu_bo_list args; > >> unsigned i; > >> int r; > >> diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c > >> index 3b8231aa..5bedf748 100644 > >> --- a/amdgpu/amdgpu_cs.c > >> +++ b/amdgpu/amdgpu_cs.c > >> @@ -724,20 +724,45 @@ drm_public int > amdgpu_cs_submit_raw(amdgpu_device_handle dev, > >> r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_CS, > >> &cs, sizeof(cs)); > >> if (r) > >> return r; > >> > >> if (seq_no) > >> *seq_no = cs.out.handle; > >> return 0; > >> } > >> > >> +drm_public int amdgpu_cs_submit_raw2(amdgpu_device_handle dev, > >> + amdgpu_context_handle context, > >> + uint32_t bo_list_handle, > >> + int num_chunks, > >> + struct drm_amdgpu_cs_chunk *chunks, > >> + uint64_t *seq_no) > >> +{ > >> + union drm_amdgpu_cs cs = {0}; > >> + uint64_t *chunk_array; > >> + int i, r; > >> + > >> + chunk_array = alloca(sizeof(uint64_t) * num_chunks); > >> + for (i = 0; i < num_chunks; i++) > >> + chunk_array[i] = (uint64_t)(uintptr_t)&chunks[i]; > >> + cs.in.chunks = (uint64_t)(uintptr_t)chunk_array; > >> + cs.in.ctx_id = context->id; > >> + cs.in.bo_list_handle = bo_list_handle; > >> + cs.in.num_chunks = num_chunks; > >> + r = drmCommandWriteRead(dev->fd, DRM_AMDGPU_CS, > >> + &cs, sizeof(cs)); > >> + if (!r && seq_no) > >> + *seq_no = cs.out.handle; > >> + return r; > >> +} > >> + > >> drm_public void amdgpu_cs_chunk_fence_info_to_data(struct > amdgpu_cs_fence_info *fence_info, > >> struct > drm_amdgpu_cs_chunk_data *data) > >> { > >> data->fence_data.handle = fence_info->handle->handle; > >> data->fence_data.offset = fence_info->offset * > sizeof(uint64_t); > >> } > >> > >> drm_public void amdgpu_cs_chunk_fence_to_dep(struct amdgpu_cs_fence > *fence, > >> struct drm_amdgpu_cs_chunk_dep > *dep) > >> { > >> -- > >> 2.17.1 > >> > >> _______________________________________________ > >> amd-gfx mailing list > >> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx > >