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

[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?

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

Dave.