[PATCH libdrm 0/2] amdgpu: add amdgpu_cs_wait_fences

[PATCH libdrm 0/2] amdgpu: add amdgpu_cs_wait_fences

[Nicolai Hähnle]

Hi all,

These changes expose a function to call the WAIT_FENCES ioctl for
waiting on multiple fences at the same time. This is useful for
Vulkan.

They are mostly changes that have been in the amdgpu-pro libdrm
for a long time. I've taken the liberty to clean them up a bit
and add some missing bits.

Please review!
Thanks,
Nicolai

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 1/2] amdgpu: add the interface of waiting multiple fences

[Nicolai Hähnle]

From: Nicolai Hähnle <nicolai.haehnle@amd.com>

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
[v2: allow returning the first signaled fence index]
Signed-off-by: monk.liu <Monk.Liu@amd.com>
[v3:
 - cleanup *status setting
 - fix amdgpu symbols check]
Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com> (v1)
---
 amdgpu/amdgpu-symbol-check |  1 +
 amdgpu/amdgpu.h            | 23 ++++++++++++++
 amdgpu/amdgpu_cs.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
index 4d1ae65..81ef9b4 100755
--- a/amdgpu/amdgpu-symbol-check
+++ b/amdgpu/amdgpu-symbol-check
@@ -26,20 +26,21 @@ amdgpu_bo_va_op_raw
 amdgpu_bo_wait_for_idle
 amdgpu_create_bo_from_user_mem
 amdgpu_cs_create_semaphore
 amdgpu_cs_ctx_create
 amdgpu_cs_ctx_free
 amdgpu_cs_destroy_semaphore
 amdgpu_cs_query_fence_status
 amdgpu_cs_query_reset_state
 amdgpu_cs_signal_semaphore
 amdgpu_cs_submit
+amdgpu_cs_wait_fences
 amdgpu_cs_wait_semaphore
 amdgpu_device_deinitialize
 amdgpu_device_initialize
 amdgpu_get_marketing_name
 amdgpu_query_buffer_size_alignment
 amdgpu_query_crtc_from_id
 amdgpu_query_firmware_version
 amdgpu_query_gds_info
 amdgpu_query_gpu_info
 amdgpu_query_heap_info
diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index 55884b2..fdea905 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -900,20 +900,43 @@ int amdgpu_cs_submit(amdgpu_context_handle context,
  *	 returned in the case if submission was completed or timeout error
  *	 code.
  *
  * \sa amdgpu_cs_submit()
 */
 int amdgpu_cs_query_fence_status(struct amdgpu_cs_fence *fence,
 				 uint64_t timeout_ns,
 				 uint64_t flags,
 				 uint32_t *expired);
 
+/**
+ *  Wait for multiple fences
+ *
+ * \param   fences      - \c [in] The fence array to wait
+ * \param   fence_count - \c [in] The fence count
+ * \param   wait_all    - \c [in] If true, wait all fences to be signaled,
+ *                                otherwise, wait at least one fence
+ * \param   timeout_ns  - \c [in] The timeout to wait, in nanoseconds
+ * \param   status      - \c [out] '1' for signaled, '0' for timeout
+ * \param   first       - \c [out] the index of the first signaled fence from @fences
+ *
+ * \return  0 on success
+ *          <0 - Negative POSIX Error code
+ *
+ * \note    Currently it supports only one amdgpu_device. All fences come from
+ *          the same amdgpu_device with the same fd.
+*/
+int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
+			  uint32_t fence_count,
+			  bool wait_all,
+			  uint64_t timeout_ns,
+			  uint32_t *status, uint32_t *first);
+
 /*
  * Query / Info API
  *
 */
 
 /**
  * Query allocation size alignments
  *
  * UMD should query information about GPU VM MC size alignments requirements
  * to be able correctly choose required allocation size and implement
diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
index fb5b3a8..707e6d1 100644
--- a/amdgpu/amdgpu_cs.c
+++ b/amdgpu/amdgpu_cs.c
@@ -436,20 +436,94 @@ int amdgpu_cs_query_fence_status(struct amdgpu_cs_fence *fence,
 	r = amdgpu_ioctl_wait_cs(fence->context, fence->ip_type,
 				fence->ip_instance, fence->ring,
 			       	fence->fence, timeout_ns, flags, &busy);
 
 	if (!r && !busy)
 		*expired = true;
 
 	return r;
 }
 
+static int amdgpu_ioctl_wait_fences(struct amdgpu_cs_fence *fences,
+				    uint32_t fence_count,
+				    bool wait_all,
+				    uint64_t timeout_ns,
+				    uint32_t *status,
+				    uint32_t *first)
+{
+	struct drm_amdgpu_fence *drm_fences;
+	amdgpu_device_handle dev = fences[0].context->dev;
+	union drm_amdgpu_wait_fences args;
+	int r;
+	uint32_t i;
+
+	drm_fences = alloca(sizeof(struct drm_amdgpu_fence) * fence_count);
+	for (i = 0; i < fence_count; i++) {
+		drm_fences[i].ctx_id = fences[i].context->id;
+		drm_fences[i].ip_type = fences[i].ip_type;
+		drm_fences[i].ip_instance = fences[i].ip_instance;
+		drm_fences[i].ring = fences[i].ring;
+		drm_fences[i].seq_no = fences[i].fence;
+	}
+
+	memset(&args, 0, sizeof(args));
+	args.in.fences = (uint64_t)(uintptr_t)drm_fences;
+	args.in.fence_count = fence_count;
+	args.in.wait_all = wait_all;
+	args.in.timeout_ns = amdgpu_cs_calculate_timeout(timeout_ns);
+
+	r = drmIoctl(dev->fd, DRM_IOCTL_AMDGPU_WAIT_FENCES, &args);
+	if (r)
+		return -errno;
+
+	*status = args.out.status;
+
+	if (first)
+		*first = args.out.first_signaled;
+
+	return 0;
+}
+
+int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
+			  uint32_t fence_count,
+			  bool wait_all,
+			  uint64_t timeout_ns,
+			  uint32_t *status,
+			  uint32_t *first)
+{
+	uint32_t i;
+	int r;
+
+	/* Sanity check */
+	if (NULL == fences)
+		return -EINVAL;
+	if (NULL == status)
+		return -EINVAL;
+	if (fence_count <= 0)
+		return -EINVAL;
+	for (i = 0; i < fence_count; i++) {
+		if (NULL == fences[i].context)
+			return -EINVAL;
+		if (fences[i].ip_type >= AMDGPU_HW_IP_NUM)
+			return -EINVAL;
+		if (fences[i].ring >= AMDGPU_CS_MAX_RINGS)
+			return -EINVAL;
+	}
+
+	*status = 0;
+
+	r = amdgpu_ioctl_wait_fences(fences, fence_count, wait_all, timeout_ns,
+				     status, first);
+
+	return r;
+}
+
 int amdgpu_cs_create_semaphore(amdgpu_semaphore_handle *sem)
 {
 	struct amdgpu_semaphore *gpu_semaphore;
 
 	if (NULL == sem)
 		return -EINVAL;
 
 	gpu_semaphore = calloc(1, sizeof(struct amdgpu_semaphore));
 	if (NULL == gpu_semaphore)
 		return -ENOMEM;
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH libdrm 2/2] amdgpu: add a test for amdgpu_cs_wait_fences

[Nicolai Hähnle]

From: Nicolai Hähnle <nicolai.haehnle@amd.com>

Signed-off-by: monk.liu <Monk.Liu@amd.com>
[v2: actually hook up the test case]
Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
---
 tests/amdgpu/basic_tests.c | 100 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/tests/amdgpu/basic_tests.c b/tests/amdgpu/basic_tests.c
index 4dce67e..8d5844b 100644
--- a/tests/amdgpu/basic_tests.c
+++ b/tests/amdgpu/basic_tests.c
@@ -38,34 +38,36 @@
 #include "amdgpu_drm.h"
 
 static  amdgpu_device_handle device_handle;
 static  uint32_t  major_version;
 static  uint32_t  minor_version;
 
 static void amdgpu_query_info_test(void);
 static void amdgpu_memory_alloc(void);
 static void amdgpu_command_submission_gfx(void);
 static void amdgpu_command_submission_compute(void);
+static void amdgpu_command_submission_multi_fence(void);
 static void amdgpu_command_submission_sdma(void);
 static void amdgpu_userptr_test(void);
 static void amdgpu_semaphore_test(void);
 
 static void amdgpu_command_submission_write_linear_helper(unsigned ip_type);
 static void amdgpu_command_submission_const_fill_helper(unsigned ip_type);
 static void amdgpu_command_submission_copy_linear_helper(unsigned ip_type);
 
 CU_TestInfo basic_tests[] = {
 	{ "Query Info Test",  amdgpu_query_info_test },
 	{ "Memory alloc Test",  amdgpu_memory_alloc },
 	{ "Userptr Test",  amdgpu_userptr_test },
 	{ "Command submission Test (GFX)",  amdgpu_command_submission_gfx },
 	{ "Command submission Test (Compute)", amdgpu_command_submission_compute },
+	{ "Command submission Test (Multi-Fence)", amdgpu_command_submission_multi_fence },
 	{ "Command submission Test (SDMA)", amdgpu_command_submission_sdma },
 	{ "SW semaphore Test",  amdgpu_semaphore_test },
 	CU_TEST_INFO_NULL,
 };
 #define BUFFER_SIZE (8 * 1024)
 #define SDMA_PKT_HEADER_op_offset 0
 #define SDMA_PKT_HEADER_op_mask   0x000000FF
 #define SDMA_PKT_HEADER_op_shift  0
 #define SDMA_PKT_HEADER_OP(x) (((x) & SDMA_PKT_HEADER_op_mask) << SDMA_PKT_HEADER_op_shift)
 #define SDMA_OPCODE_CONSTANT_FILL  11
@@ -1142,20 +1144,118 @@ static void amdgpu_command_submission_sdma_copy_linear(void)
 	amdgpu_command_submission_copy_linear_helper(AMDGPU_HW_IP_DMA);
 }
 
 static void amdgpu_command_submission_sdma(void)
 {
 	amdgpu_command_submission_sdma_write_linear();
 	amdgpu_command_submission_sdma_const_fill();
 	amdgpu_command_submission_sdma_copy_linear();
 }
 
+static void amdgpu_command_submission_multi_fence_wait_all(bool wait_all)
+{
+	amdgpu_context_handle context_handle;
+	amdgpu_bo_handle ib_result_handle, ib_result_ce_handle;
+	void *ib_result_cpu, *ib_result_ce_cpu;
+	uint64_t ib_result_mc_address, ib_result_ce_mc_address;
+	struct amdgpu_cs_request ibs_request[2] = {0};
+	struct amdgpu_cs_ib_info ib_info[2];
+	struct amdgpu_cs_fence fence_status[2] = {0};
+	uint32_t *ptr;
+	uint32_t expired;
+	amdgpu_bo_list_handle bo_list;
+	amdgpu_va_handle va_handle, va_handle_ce;
+	int r;
+	int i, ib_cs_num = 2;
+
+	r = amdgpu_cs_ctx_create(device_handle, &context_handle);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_bo_alloc_and_map(device_handle, 4096, 4096,
+				    AMDGPU_GEM_DOMAIN_GTT, 0,
+				    &ib_result_handle, &ib_result_cpu,
+				    &ib_result_mc_address, &va_handle);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_bo_alloc_and_map(device_handle, 4096, 4096,
+				    AMDGPU_GEM_DOMAIN_GTT, 0,
+				    &ib_result_ce_handle, &ib_result_ce_cpu,
+				    &ib_result_ce_mc_address, &va_handle_ce);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_get_bo_list(device_handle, ib_result_handle,
+			       ib_result_ce_handle, &bo_list);
+	CU_ASSERT_EQUAL(r, 0);
+
+	memset(ib_info, 0, 2 * sizeof(struct amdgpu_cs_ib_info));
+
+	/* IT_SET_CE_DE_COUNTERS */
+	ptr = ib_result_ce_cpu;
+	ptr[0] = 0xc0008900;
+	ptr[1] = 0;
+	ptr[2] = 0xc0008400;
+	ptr[3] = 1;
+	ib_info[0].ib_mc_address = ib_result_ce_mc_address;
+	ib_info[0].size = 4;
+	ib_info[0].flags = AMDGPU_IB_FLAG_CE;
+
+	/* IT_WAIT_ON_CE_COUNTER */
+	ptr = ib_result_cpu;
+	ptr[0] = 0xc0008600;
+	ptr[1] = 0x00000001;
+	ib_info[1].ib_mc_address = ib_result_mc_address;
+	ib_info[1].size = 2;
+
+	for (i = 0; i < ib_cs_num; i++) {
+		ibs_request[i].ip_type = AMDGPU_HW_IP_GFX;
+		ibs_request[i].number_of_ibs = 2;
+		ibs_request[i].ibs = ib_info;
+		ibs_request[i].resources = bo_list;
+		ibs_request[i].fence_info.handle = NULL;
+	}
+
+	r = amdgpu_cs_submit(context_handle, 0,ibs_request, ib_cs_num);
+
+	CU_ASSERT_EQUAL(r, 0);
+
+	for (i = 0; i < ib_cs_num; i++) {
+		fence_status[i].context = context_handle;
+		fence_status[i].ip_type = AMDGPU_HW_IP_GFX;
+		fence_status[i].fence = ibs_request[i].seq_no;
+	}
+
+	r = amdgpu_cs_wait_fences(fence_status, ib_cs_num, wait_all,
+				AMDGPU_TIMEOUT_INFINITE,
+				&expired, NULL);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_bo_unmap_and_free(ib_result_handle, va_handle,
+				     ib_result_mc_address, 4096);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_bo_unmap_and_free(ib_result_ce_handle, va_handle_ce,
+				     ib_result_ce_mc_address, 4096);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_bo_list_destroy(bo_list);
+	CU_ASSERT_EQUAL(r, 0);
+
+	r = amdgpu_cs_ctx_free(context_handle);
+	CU_ASSERT_EQUAL(r, 0);
+}
+
+static void amdgpu_command_submission_multi_fence(void)
+{
+	amdgpu_command_submission_multi_fence_wait_all(true);
+	amdgpu_command_submission_multi_fence_wait_all(false);
+}
+
 static void amdgpu_userptr_test(void)
 {
 	int i, r, j;
 	uint32_t *pm4 = NULL;
 	uint64_t bo_mc;
 	void *ptr = NULL;
 	int pm4_dw = 256;
 	int sdma_write_length = 4;
 	amdgpu_bo_handle handle;
 	amdgpu_context_handle context_handle;
-- 
2.9.3

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 0/2] amdgpu: add amdgpu_cs_wait_fences

[Nicolai Hähnle]

Post-Easter ping - anybody want to give their R-b?

Thanks,
Nicolai

On 13.04.2017 16:47, Nicolai Hähnle wrote:
> Hi all,
>
> These changes expose a function to call the WAIT_FENCES ioctl for
> waiting on multiple fences at the same time. This is useful for
> Vulkan.
>
> They are mostly changes that have been in the amdgpu-pro libdrm
> for a long time. I've taken the liberty to clean them up a bit
> and add some missing bits.
>
> Please review!
> Thanks,
> Nicolai
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 0/2] amdgpu: add amdgpu_cs_wait_fences

[Alex Deucher]

On Tue, Apr 18, 2017 at 6:11 AM, Nicolai Hähnle <nhaehnle@gmail.com> wrote:
> Post-Easter ping - anybody want to give their R-b?

For the series:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

>
> Thanks,
> Nicolai
>
> On 13.04.2017 16:47, Nicolai Hähnle wrote:
>>
>> Hi all,
>>
>> These changes expose a function to call the WAIT_FENCES ioctl for
>> waiting on multiple fences at the same time. This is useful for
>> Vulkan.
>>
>> They are mostly changes that have been in the amdgpu-pro libdrm
>> for a long time. I've taken the liberty to clean them up a bit
>> and add some missing bits.
>>
>> Please review!
>> Thanks,
>> Nicolai
>>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> Aber vergiss niemals, wie sie sein sollte.
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/2] amdgpu: add the interface of waiting multiple fences

[Edward O'Callaghan]

[-- Attachment #1.1.1: Type: text/plain, Size: 6592 bytes --]



On 04/14/2017 12:47 AM, Nicolai Hähnle wrote:
> From: Nicolai Hähnle <nicolai.haehnle-5C7GfCeVMHo@public.gmane.org>
> 
> Signed-off-by: Junwei Zhang <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>
> [v2: allow returning the first signaled fence index]
> Signed-off-by: monk.liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org>
> [v3:
>  - cleanup *status setting
>  - fix amdgpu symbols check]
> Signed-off-by: Nicolai Hähnle <nicolai.haehnle-5C7GfCeVMHo@public.gmane.org>
> Reviewed-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org> (v1)
> Reviewed-by: Jammy Zhou <Jammy.Zhou-5C7GfCeVMHo@public.gmane.org> (v1)
> ---
>  amdgpu/amdgpu-symbol-check |  1 +
>  amdgpu/amdgpu.h            | 23 ++++++++++++++
>  amdgpu/amdgpu_cs.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
> 
> diff --git a/amdgpu/amdgpu-symbol-check b/amdgpu/amdgpu-symbol-check
> index 4d1ae65..81ef9b4 100755
> --- a/amdgpu/amdgpu-symbol-check
> +++ b/amdgpu/amdgpu-symbol-check
> @@ -26,20 +26,21 @@ amdgpu_bo_va_op_raw
>  amdgpu_bo_wait_for_idle
>  amdgpu_create_bo_from_user_mem
>  amdgpu_cs_create_semaphore
>  amdgpu_cs_ctx_create
>  amdgpu_cs_ctx_free
>  amdgpu_cs_destroy_semaphore
>  amdgpu_cs_query_fence_status
>  amdgpu_cs_query_reset_state
>  amdgpu_cs_signal_semaphore
>  amdgpu_cs_submit
> +amdgpu_cs_wait_fences
>  amdgpu_cs_wait_semaphore
>  amdgpu_device_deinitialize
>  amdgpu_device_initialize
>  amdgpu_get_marketing_name
>  amdgpu_query_buffer_size_alignment
>  amdgpu_query_crtc_from_id
>  amdgpu_query_firmware_version
>  amdgpu_query_gds_info
>  amdgpu_query_gpu_info
>  amdgpu_query_heap_info
> diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
> index 55884b2..fdea905 100644
> --- a/amdgpu/amdgpu.h
> +++ b/amdgpu/amdgpu.h
> @@ -900,20 +900,43 @@ int amdgpu_cs_submit(amdgpu_context_handle context,
>   *	 returned in the case if submission was completed or timeout error
>   *	 code.
>   *
>   * \sa amdgpu_cs_submit()
>  */
>  int amdgpu_cs_query_fence_status(struct amdgpu_cs_fence *fence,
>  				 uint64_t timeout_ns,
>  				 uint64_t flags,
>  				 uint32_t *expired);
>  
> +/**
> + *  Wait for multiple fences
> + *
> + * \param   fences      - \c [in] The fence array to wait
> + * \param   fence_count - \c [in] The fence count
> + * \param   wait_all    - \c [in] If true, wait all fences to be signaled,
> + *                                otherwise, wait at least one fence
> + * \param   timeout_ns  - \c [in] The timeout to wait, in nanoseconds
> + * \param   status      - \c [out] '1' for signaled, '0' for timeout
> + * \param   first       - \c [out] the index of the first signaled fence from @fences
> + *
> + * \return  0 on success
> + *          <0 - Negative POSIX Error code
> + *
> + * \note    Currently it supports only one amdgpu_device. All fences come from
> + *          the same amdgpu_device with the same fd.
> +*/
> +int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
> +			  uint32_t fence_count,
> +			  bool wait_all,
> +			  uint64_t timeout_ns,
> +			  uint32_t *status, uint32_t *first);
> +
>  /*
>   * Query / Info API
>   *
>  */
>  
>  /**
>   * Query allocation size alignments
>   *
>   * UMD should query information about GPU VM MC size alignments requirements
>   * to be able correctly choose required allocation size and implement
> diff --git a/amdgpu/amdgpu_cs.c b/amdgpu/amdgpu_cs.c
> index fb5b3a8..707e6d1 100644
> --- a/amdgpu/amdgpu_cs.c
> +++ b/amdgpu/amdgpu_cs.c
> @@ -436,20 +436,94 @@ int amdgpu_cs_query_fence_status(struct amdgpu_cs_fence *fence,
>  	r = amdgpu_ioctl_wait_cs(fence->context, fence->ip_type,
>  				fence->ip_instance, fence->ring,
>  			       	fence->fence, timeout_ns, flags, &busy);
>  
>  	if (!r && !busy)
>  		*expired = true;
>  
>  	return r;
>  }
>  
> +static int amdgpu_ioctl_wait_fences(struct amdgpu_cs_fence *fences,
> +				    uint32_t fence_count,
> +				    bool wait_all,
> +				    uint64_t timeout_ns,
> +				    uint32_t *status,
> +				    uint32_t *first)
> +{
> +	struct drm_amdgpu_fence *drm_fences;
> +	amdgpu_device_handle dev = fences[0].context->dev;
> +	union drm_amdgpu_wait_fences args;
> +	int r;
> +	uint32_t i;
> +
> +	drm_fences = alloca(sizeof(struct drm_amdgpu_fence) * fence_count);
> +	for (i = 0; i < fence_count; i++) {
> +		drm_fences[i].ctx_id = fences[i].context->id;
> +		drm_fences[i].ip_type = fences[i].ip_type;
> +		drm_fences[i].ip_instance = fences[i].ip_instance;
> +		drm_fences[i].ring = fences[i].ring;
> +		drm_fences[i].seq_no = fences[i].fence;
> +	}
> +
> +	memset(&args, 0, sizeof(args));
> +	args.in.fences = (uint64_t)(uintptr_t)drm_fences;
> +	args.in.fence_count = fence_count;
> +	args.in.wait_all = wait_all;
> +	args.in.timeout_ns = amdgpu_cs_calculate_timeout(timeout_ns);
> +
> +	r = drmIoctl(dev->fd, DRM_IOCTL_AMDGPU_WAIT_FENCES, &args);
> +	if (r)
> +		return -errno;

Hi Nicolai,

you will leak drm_fences here on the error branch.

> +
> +	*status = args.out.status;
> +
> +	if (first)
> +		*first = args.out.first_signaled;
> +
> +	return 0;
> +}
> +
> +int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
> +			  uint32_t fence_count,
> +			  bool wait_all,
> +			  uint64_t timeout_ns,
> +			  uint32_t *status,
> +			  uint32_t *first)
> +{
> +	uint32_t i;
> +	int r;

no need for a intermediate ret, just return amdgpu_ioctl_wait_fences()
directly?

> +
> +	/* Sanity check */
> +	if (NULL == fences)
> +		return -EINVAL;
> +	if (NULL == status)
> +		return -EINVAL;
> +	if (fence_count <= 0)
> +		return -EINVAL;

may as well combine these branches?

if (!fences || !status || !fence_count)
	return -EINVAL;

as fence_count is unsigned.

Kind Regards,
Edward.

> +	for (i = 0; i < fence_count; i++) {
> +		if (NULL == fences[i].context)
> +			return -EINVAL;
> +		if (fences[i].ip_type >= AMDGPU_HW_IP_NUM)
> +			return -EINVAL;
> +		if (fences[i].ring >= AMDGPU_CS_MAX_RINGS)
> +			return -EINVAL;
> +	}
> +
> +	*status = 0;
> +
> +	r = amdgpu_ioctl_wait_fences(fences, fence_count, wait_all, timeout_ns,
> +				     status, first);
> +
> +	return r;
> +}
> +
>  int amdgpu_cs_create_semaphore(amdgpu_semaphore_handle *sem)
>  {
>  	struct amdgpu_semaphore *gpu_semaphore;
>  
>  	if (NULL == sem)
>  		return -EINVAL;
>  
>  	gpu_semaphore = calloc(1, sizeof(struct amdgpu_semaphore));
>  	if (NULL == gpu_semaphore)
>  		return -ENOMEM;
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/2] amdgpu: add the interface of waiting multiple fences

[Nicolai Hähnle]

On 18.04.2017 17:47, Edward O'Callaghan wrote:
> On 04/14/2017 12:47 AM, Nicolai Hähnle wrote:
>> From: Nicolai Hähnle <nicolai.haehnle@amd.com>
>>
>> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
>> [v2: allow returning the first signaled fence index]
>> Signed-off-by: monk.liu <Monk.Liu@amd.com>
>> [v3:
>>  - cleanup *status setting
>>  - fix amdgpu symbols check]
>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com> (v1)
>> Reviewed-by: Jammy Zhou <Jammy.Zhou@amd.com> (v1)
>> ---
>>  amdgpu/amdgpu-symbol-check |  1 +
>>  amdgpu/amdgpu.h            | 23 ++++++++++++++
>>  amdgpu/amdgpu_cs.c         | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 98 insertions(+)
>>
[snip]
>> +static int amdgpu_ioctl_wait_fences(struct amdgpu_cs_fence *fences,
>> +				    uint32_t fence_count,
>> +				    bool wait_all,
>> +				    uint64_t timeout_ns,
>> +				    uint32_t *status,
>> +				    uint32_t *first)
>> +{
>> +	struct drm_amdgpu_fence *drm_fences;
>> +	amdgpu_device_handle dev = fences[0].context->dev;
>> +	union drm_amdgpu_wait_fences args;
>> +	int r;
>> +	uint32_t i;
>> +
>> +	drm_fences = alloca(sizeof(struct drm_amdgpu_fence) * fence_count);
>> +	for (i = 0; i < fence_count; i++) {
>> +		drm_fences[i].ctx_id = fences[i].context->id;
>> +		drm_fences[i].ip_type = fences[i].ip_type;
>> +		drm_fences[i].ip_instance = fences[i].ip_instance;
>> +		drm_fences[i].ring = fences[i].ring;
>> +		drm_fences[i].seq_no = fences[i].fence;
>> +	}
>> +
>> +	memset(&args, 0, sizeof(args));
>> +	args.in.fences = (uint64_t)(uintptr_t)drm_fences;
>> +	args.in.fence_count = fence_count;
>> +	args.in.wait_all = wait_all;
>> +	args.in.timeout_ns = amdgpu_cs_calculate_timeout(timeout_ns);
>> +
>> +	r = drmIoctl(dev->fd, DRM_IOCTL_AMDGPU_WAIT_FENCES, &args);
>> +	if (r)
>> +		return -errno;
>
> Hi Nicolai,
>
> you will leak drm_fences here on the error branch.

It's an alloca, so it's automatically freed when the function returns.


>> +
>> +	*status = args.out.status;
>> +
>> +	if (first)
>> +		*first = args.out.first_signaled;
>> +
>> +	return 0;
>> +}
>> +
>> +int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
>> +			  uint32_t fence_count,
>> +			  bool wait_all,
>> +			  uint64_t timeout_ns,
>> +			  uint32_t *status,
>> +			  uint32_t *first)
>> +{
>> +	uint32_t i;
>> +	int r;
>
> no need for a intermediate ret, just return amdgpu_ioctl_wait_fences()
> directly?

Good point, I'll change that before I push.


>> +
>> +	/* Sanity check */
>> +	if (NULL == fences)
>> +		return -EINVAL;
>> +	if (NULL == status)
>> +		return -EINVAL;
>> +	if (fence_count <= 0)
>> +		return -EINVAL;
>
> may as well combine these branches?
>
> if (!fences || !status || !fence_count)
> 	return -EINVAL;
>
> as fence_count is unsigned.

Yeah, that makes some sense, but I decided to keep the separate 
if-statements because other functions are written like this as well.

Thanks,
Nicolai



>
> Kind Regards,
> Edward.
>
>> +	for (i = 0; i < fence_count; i++) {
>> +		if (NULL == fences[i].context)
>> +			return -EINVAL;
>> +		if (fences[i].ip_type >= AMDGPU_HW_IP_NUM)
>> +			return -EINVAL;
>> +		if (fences[i].ring >= AMDGPU_CS_MAX_RINGS)
>> +			return -EINVAL;
>> +	}
>> +
>> +	*status = 0;
>> +
>> +	r = amdgpu_ioctl_wait_fences(fences, fence_count, wait_all, timeout_ns,
>> +				     status, first);
>> +
>> +	return r;
>> +}
>> +
>>  int amdgpu_cs_create_semaphore(amdgpu_semaphore_handle *sem)
>>  {
>>  	struct amdgpu_semaphore *gpu_semaphore;
>>
>>  	if (NULL == sem)
>>  		return -EINVAL;
>>
>>  	gpu_semaphore = calloc(1, sizeof(struct amdgpu_semaphore));
>>  	if (NULL == gpu_semaphore)
>>  		return -ENOMEM;
>>
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH libdrm 1/2] amdgpu: add the interface of waiting multiple fences

[Edward O'Callaghan]

[-- Attachment #1.1.1: Type: text/plain, Size: 4680 bytes --]



On 04/19/2017 04:28 AM, Nicolai Hähnle wrote:
> On 18.04.2017 17:47, Edward O'Callaghan wrote:
>> On 04/14/2017 12:47 AM, Nicolai Hähnle wrote:
>>> From: Nicolai Hähnle <nicolai.haehnle-5C7GfCeVMHo@public.gmane.org>
>>>
>>> Signed-off-by: Junwei Zhang <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>
>>> [v2: allow returning the first signaled fence index]
>>> Signed-off-by: monk.liu <Monk.Liu-5C7GfCeVMHo@public.gmane.org>
>>> [v3:
>>>  - cleanup *status setting
>>>  - fix amdgpu symbols check]
>>> Signed-off-by: Nicolai Hähnle <nicolai.haehnle-5C7GfCeVMHo@public.gmane.org>
>>> Reviewed-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org> (v1)
>>> Reviewed-by: Jammy Zhou <Jammy.Zhou-5C7GfCeVMHo@public.gmane.org> (v1)
>>> ---
>>>  amdgpu/amdgpu-symbol-check |  1 +
>>>  amdgpu/amdgpu.h            | 23 ++++++++++++++
>>>  amdgpu/amdgpu_cs.c         | 74
>>> ++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 98 insertions(+)
>>>
> [snip]
>>> +static int amdgpu_ioctl_wait_fences(struct amdgpu_cs_fence *fences,
>>> +                    uint32_t fence_count,
>>> +                    bool wait_all,
>>> +                    uint64_t timeout_ns,
>>> +                    uint32_t *status,
>>> +                    uint32_t *first)
>>> +{
>>> +    struct drm_amdgpu_fence *drm_fences;
>>> +    amdgpu_device_handle dev = fences[0].context->dev;
>>> +    union drm_amdgpu_wait_fences args;
>>> +    int r;
>>> +    uint32_t i;
>>> +
>>> +    drm_fences = alloca(sizeof(struct drm_amdgpu_fence) * fence_count);
>>> +    for (i = 0; i < fence_count; i++) {
>>> +        drm_fences[i].ctx_id = fences[i].context->id;
>>> +        drm_fences[i].ip_type = fences[i].ip_type;
>>> +        drm_fences[i].ip_instance = fences[i].ip_instance;
>>> +        drm_fences[i].ring = fences[i].ring;
>>> +        drm_fences[i].seq_no = fences[i].fence;
>>> +    }
>>> +
>>> +    memset(&args, 0, sizeof(args));
>>> +    args.in.fences = (uint64_t)(uintptr_t)drm_fences;
>>> +    args.in.fence_count = fence_count;
>>> +    args.in.wait_all = wait_all;
>>> +    args.in.timeout_ns = amdgpu_cs_calculate_timeout(timeout_ns);
>>> +
>>> +    r = drmIoctl(dev->fd, DRM_IOCTL_AMDGPU_WAIT_FENCES, &args);
>>> +    if (r)
>>> +        return -errno;
>>
>> Hi Nicolai,
>>
>> you will leak drm_fences here on the error branch.
> 
> It's an alloca, so it's automatically freed when the function returns.
> 
> 
>>> +
>>> +    *status = args.out.status;
>>> +
>>> +    if (first)
>>> +        *first = args.out.first_signaled;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int amdgpu_cs_wait_fences(struct amdgpu_cs_fence *fences,
>>> +              uint32_t fence_count,
>>> +              bool wait_all,
>>> +              uint64_t timeout_ns,
>>> +              uint32_t *status,
>>> +              uint32_t *first)
>>> +{
>>> +    uint32_t i;
>>> +    int r;
>>
>> no need for a intermediate ret, just return amdgpu_ioctl_wait_fences()
>> directly?
> 
> Good point, I'll change that before I push.
> 
> 
>>> +
>>> +    /* Sanity check */
>>> +    if (NULL == fences)
>>> +        return -EINVAL;
>>> +    if (NULL == status)
>>> +        return -EINVAL;
>>> +    if (fence_count <= 0)
>>> +        return -EINVAL;
>>
>> may as well combine these branches?
>>
>> if (!fences || !status || !fence_count)
>>     return -EINVAL;
>>
>> as fence_count is unsigned.
> 
> Yeah, that makes some sense, but I decided to keep the separate
> if-statements because other functions are written like this as well.

Its not completely consistent actually, I sent in a patch last night to
fix the rest.

Cheers,
Edward.

> 
> Thanks,
> Nicolai
> 
> 
> 
>>
>> Kind Regards,
>> Edward.
>>
>>> +    for (i = 0; i < fence_count; i++) {
>>> +        if (NULL == fences[i].context)
>>> +            return -EINVAL;
>>> +        if (fences[i].ip_type >= AMDGPU_HW_IP_NUM)
>>> +            return -EINVAL;
>>> +        if (fences[i].ring >= AMDGPU_CS_MAX_RINGS)
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    *status = 0;
>>> +
>>> +    r = amdgpu_ioctl_wait_fences(fences, fence_count, wait_all,
>>> timeout_ns,
>>> +                     status, first);
>>> +
>>> +    return r;
>>> +}
>>> +
>>>  int amdgpu_cs_create_semaphore(amdgpu_semaphore_handle *sem)
>>>  {
>>>      struct amdgpu_semaphore *gpu_semaphore;
>>>
>>>      if (NULL == sem)
>>>          return -EINVAL;
>>>
>>>      gpu_semaphore = calloc(1, sizeof(struct amdgpu_semaphore));
>>>      if (NULL == gpu_semaphore)
>>>          return -ENOMEM;
>>>
>>
> 
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx