Re: [PATCH 4/6] android: convert sync to fence api, v4

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

op 04-03-14 11:00, Daniel Vetter schreef:
> On Tue, Mar 04, 2014 at 09:20:58AM +0100, Maarten Lankhorst wrote:
>> op 04-03-14 09:14, Daniel Vetter schreef:
>>> On Tue, Mar 04, 2014 at 08:50:38AM +0100, Maarten Lankhorst wrote:
>>>> op 03-03-14 22:11, Daniel Vetter schreef:
>>>>> On Mon, Feb 17, 2014 at 04:57:19PM +0100, Maarten Lankhorst wrote:
>>>>>> Android syncpoints can be mapped to a timeline. This removes the need
>>>>>> to maintain a separate api for synchronization. I've left the android
>>>>>> trace events in place, but the core fence events should already be
>>>>>> sufficient for debugging.
>>>>>>
>>>>>> v2:
>>>>>> - Call fence_remove_callback in sync_fence_free if not all fences have fired.
>>>>>> v3:
>>>>>> - Merge Colin Cross' bugfixes, and the android fence merge optimization.
>>>>>> v4:
>>>>>> - Merge with the upstream fixes.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>>>>> ---
>>>>> Snipped everything but headers - Ian Lister from our android team is
>>>>> signed up to have a more in-depth look at proper integration with android
>>>>> syncpoints. Adding him to cc.
>>>>>
>>>>>> diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
>>>>>> index 62e2255b1c1e..6036dbdc8e6f 100644
>>>>>> --- a/drivers/staging/android/sync.h
>>>>>> +++ b/drivers/staging/android/sync.h
>>>>>> @@ -21,6 +21,7 @@
>>>>>>   #include <linux/list.h>
>>>>>>   #include <linux/spinlock.h>
>>>>>>   #include <linux/wait.h>
>>>>>> +#include <linux/fence.h>
>>>>>>
>>>>>>   struct sync_timeline;
>>>>>>   struct sync_pt;
>>>>>> @@ -40,8 +41,6 @@ struct sync_fence;
>>>>>>    * -1 if a will signal before b
>>>>>>    * @free_pt: called before sync_pt is freed
>>>>>>    * @release_obj: called before sync_timeline is freed
>>>>>> - * @print_obj: deprecated
>>>>>> - * @print_pt: deprecated
>>>>>>    * @fill_driver_data: write implementation specific driver data to data.
>>>>>>    *  should return an error if there is not enough room
>>>>>>    *  as specified by size.  This information is returned
>>>>>> @@ -67,13 +66,6 @@ struct sync_timeline_ops {
>>>>>>    /* optional */
>>>>>>    void (*release_obj)(struct sync_timeline *sync_timeline);
>>>>>>
>>>>>> - /* deprecated */
>>>>>> - void (*print_obj)(struct seq_file *s,
>>>>>> -  struct sync_timeline *sync_timeline);
>>>>>> -
>>>>>> - /* deprecated */
>>>>>> - void (*print_pt)(struct seq_file *s, struct sync_pt *sync_pt);
>>>>>> -
>>>>>>    /* optional */
>>>>>>    int (*fill_driver_data)(struct sync_pt *syncpt, void *data, int size);
>>>>>>
>>>>>> @@ -104,42 +96,48 @@ struct sync_timeline {
>>>>>>
>>>>>>    /* protected by child_list_lock */
>>>>>>    bool destroyed;
>>>>>> + int context, value;
>>>>>>
>>>>>>    struct list_head child_list_head;
>>>>>>    spinlock_t child_list_lock;
>>>>>>
>>>>>>    struct list_head active_list_head;
>>>>>> - spinlock_t active_list_lock;
>>>>>>
>>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>>>    struct list_head sync_timeline_list;
>>>>>> +#endif
>>>>>>   };
>>>>>>
>>>>>>   /**
>>>>>>    * struct sync_pt - sync point
>>>>>> - * @parent: sync_timeline to which this sync_pt belongs
>>>>>> + * @fence: base fence class
>>>>>>    * @child_list: membership in sync_timeline.child_list_head
>>>>>>    * @active_list: membership in sync_timeline.active_list_head
>>>>>> +<<<<<<< current
>>>>>>    * @signaled_list: membership in temporary signaled_list on stack
>>>>>>    * @fence: sync_fence to which the sync_pt belongs
>>>>>>    * @pt_list: membership in sync_fence.pt_list_head
>>>>>>    * @status: 1: signaled, 0:active, <0: error
>>>>>>    * @timestamp: time which sync_pt status transitioned from active to
>>>>>>    *  signaled or error.
>>>>>> +=======
>>>>>> +>>>>>>> patched
>>>>> Conflict markers ...
>>>> Oops.
>>>>>>    */
>>>>>>   struct sync_pt {
>>>>>> - struct sync_timeline *parent;
>>>>>> - struct list_head child_list;
>>>>>> + struct fence base;
>>>>> Hm, embedding feels wrong, since that still means that I'll need to
>>>>> implement two kinds of fences in i915 - one using the seqno fence to make
>>>>> dma-buf sync work, and one to implmenent sync_pt to make the android folks
>>>>> happy.
>>>>>
>>>>> If I can dream I think we should have a pointer to an underlying fence
>>>>> here, i.e. a struct sync_pt would just be a userspace interface wrapper to
>>>>> do explicit syncing using native fences, instead of implicit syncing like
>>>>> with dma-bufs. But this is all drive-by comments from a very cursory
>>>>> high-level look. I might be full of myself again ;-)
>>>>> -Daniel
>>>>>
>>>> No, the idea is that because android syncpoint is simply another type of
>>>> dma-fence, that if you deal with normal fences then android can
>>>> automatically be handled too. The userspace fence api android exposes
>>>> could be very easily made to work for dma-fence, just pass a dma-fence
>>>> to sync_fence_create.
>>>> So exposing dma-fence would probably work for android too.
>>> Hm, then why do we still have struct sync_pt around? Since it's just the
>>> internal bit, with the userspace facing object being struct sync_fence,
>>> I'd opt to shuffle any useful features into the core struct fence.
>>> -Daniel
>> To keep compatibility with the android api. I think that gradually converting them is going to be
>> more useful than to force all drivers to use a new api all at once. They could keep android
>> syncpoint api for exporting, as long as they accept dma-fence for importing/waiting.
> We don't have any users of the android sync_pt stuff (outside of the
> framework itself). So any considerations for existing drivers for
> upstreaming are imo moot. At least for the in-kernel interfaces used. For
> the actual userspace interface I guess keeping the android syncpt ioctls
> as-is has value, at least if we conclude that their not badly broken. In
> which case we need to fix that before moving it out of staging.
Any driver has to implement sync_timeline_ops though, which interacts with sync_pt?
And creating a userspace fence requires a sync_pt, although it could very trivially be changed to require a fence too.

~Maarten