On 4 April 2017 at 21:05, Christian König <deathsimple@xxxxxxxxxxx> 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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>
>>> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
