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 <[email protected]> >>> >>> Signed-off-by: Junwei Zhang <[email protected]> >>> [v2: allow returning the first signaled fence index] >>> Signed-off-by: monk.liu <[email protected]> >>> [v3: >>> - cleanup *status setting >>> - fix amdgpu symbols check] >>> Signed-off-by: Nicolai Hähnle <[email protected]> >>> Reviewed-by: Christian König <[email protected]> (v1) >>> Reviewed-by: Jammy Zhou <[email protected]> (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;
>>>
>>
>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
