[PATCH 8/8] amdgpu: use sync file for shared semaphores (v2)

[Christian König]

Am 11.04.2017 um 05:18 schrieb Dave Airlie:
> On 4 April 2017 at 21:05, Christian König <deathsimple at vodafone.de> wrote:
>> Am 04.04.2017 um 10:10 schrieb Daniel Vetter:
>>> On Tue, Apr 04, 2017 at 09:40:57AM +0200, Christian König wrote:
>>>> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>>>> From: Dave Airlie <airlied at redhat.com>
>>>>>
>>>>> This creates a new interface for amdgpu with ioctls to
>>>>> create/destroy/import and export shared semaphores using
>>>>> sem object backed by the sync_file code. The semaphores
>>>>> are not installed as fd (except for export), but rather
>>>>> like other driver internal objects in an idr. The idr
>>>>> holds the initial reference on the sync file.
>>>>>
>>>>> The command submission interface is enhanced with two new
>>>>> chunks, one for semaphore waiting, one for semaphore signalling
>>>>> and just takes a list of handles for each.
>>>>>
>>>>> This is based on work originally done by David Zhou at AMD,
>>>>> with input from Christian Konig on what things should look like.
>>>>>
>>>>> NOTE: this interface addition needs a version bump to expose
>>>>> it to userspace.
>>>>>
>>>>> v1.1: keep file reference on import.
>>>>> v2: move to using syncobjs
>>>>>
>>>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>>> Looks good to me in general, but one note below.
>>>>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  | 86
>>>>> ++++++++++++++++++++++++++++++++-
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
>>>>>     include/uapi/drm/amdgpu_drm.h           |  6 +++
>>>>>     3 files changed, 92 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 4671432..4dd210b 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -27,6 +27,7 @@
>>>>>     #include <linux/pagemap.h>
>>>>>     #include <drm/drmP.h>
>>>>>     #include <drm/amdgpu_drm.h>
>>>>> +#include <drm/drm_syncobj.h>
>>>>>     #include "amdgpu.h"
>>>>>     #include "amdgpu_trace.h"
>>>>> @@ -217,6 +218,8 @@ int amdgpu_cs_parser_init(struct amdgpu_cs_parser
>>>>> *p, void *data)
>>>>>                          break;
>>>>>                  case AMDGPU_CHUNK_ID_DEPENDENCIES:
>>>>> +               case AMDGPU_CHUNK_ID_SEM_WAIT:
>>>>> +               case AMDGPU_CHUNK_ID_SEM_SIGNAL:
>>>>>                          break;
>>>>>                  default:
>>>>> @@ -1009,6 +1012,44 @@ static int amdgpu_process_fence_dep(struct
>>>>> amdgpu_device *adev,
>>>>>          return 0;
>>>>>     }
>>>>> +static int amdgpu_sem_lookup_and_sync(struct amdgpu_device *adev,
>>>>> +                                     struct drm_file *file_private,
>>>>> +                                     struct amdgpu_sync *sync,
>>>>> +                                     uint32_t handle)
>>>>> +{
>>>>> +       int r;
>>>>> +       struct dma_fence *old_fence;
>>>>> +       r = drm_syncobj_swap_fences(file_private, handle, NULL,
>>>>> &old_fence);
>>>> What happens when we the CS fails later on? Would the semaphore then not
>>>> contain the fence any more?
>>> Atm rules are that you're not allowed to publish a dma_fence before you're
>>> committed to executing whatever it represents (i.e. guaranteed to get
>>> signalled).
>>
>> Yeah, I know. But this isn't the signaling case, but rather the waiting
>> case.
>>
>> If I'm not completely mistaken what Dave does here is retrieving the fence
>> from a semaphore object we need to wait for and setting it to NULL.
>>
>> If the command submission fails with -ERESTARTSYS after this is done the
>> retried IOCTL will behave incorrectly.
> I don't think it will since the return ioctl will already have waited won't it?

No, it the IOCTL doesn't wait for anything. If you added a wait here 
that would clearly be a NAK for the patch.

> So once it's done the waiting, there is no point in waiting on resubmit.

We don't wait in the IOCTL, instead all the fences are put as 
dependencies into the job and submit that to the GPU scheduler for 
execution when all fences are done.

Christian.

>
> Dave.