[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4

[Zhou, David(ChunMing)]

> -----Original Message-----
> From: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> Sent: Saturday, March 30, 2019 10:09 PM
> To: Koenig, Christian <Christian.Koenig at amd.com>; Zhou, David(ChunMing)
> <David1.Zhou at amd.com>; dri-devel at lists.freedesktop.org; amd-
> gfx at lists.freedesktop.org; jason at jlekstrand.net; Hector, Tobias
> <Tobias.Hector at amd.com>
> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
> interface v4
> 
> On 28/03/2019 15:18, Christian König wrote:
> > Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
> >> On 25/03/2019 08:32, Chunming Zhou wrote:
> >>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> >>>
> >>> Use the dma_fence_chain object to create a timeline of fence objects
> >>> instead of just replacing the existing fence.
> >>>
> >>> v2: rebase and cleanup
> >>> v3: fix garbage collection parameters
> >>> v4: add unorder point check, print a warn calltrace
> >>>
> >>> Signed-off-by: Christian König <christian.koenig at amd.com>
> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> >>> ---
> >>>   drivers/gpu/drm/drm_syncobj.c | 39
> >>> +++++++++++++++++++++++++++++++++++
> >>>   include/drm/drm_syncobj.h     |  5 +++++
> >>>   2 files changed, 44 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_syncobj.c
> >>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
> >>> 100644
> >>> --- a/drivers/gpu/drm/drm_syncobj.c
> >>> +++ b/drivers/gpu/drm/drm_syncobj.c
> >>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
> >>> drm_syncobj *syncobj,
> >>>       spin_unlock(&syncobj->lock);
> >>>   }
> >>>   +/**
> >>> + * drm_syncobj_add_point - add new timeline point to the syncobj
> >>> + * @syncobj: sync object to add timeline point do
> >>> + * @chain: chain node to use to add the point
> >>> + * @fence: fence to encapsulate in the chain node
> >>> + * @point: sequence number to use for the point
> >>> + *
> >>> + * Add the chain node as new timeline point to the syncobj.
> >>> + */
> >>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> >>> +               struct dma_fence_chain *chain,
> >>> +               struct dma_fence *fence,
> >>> +               uint64_t point)
> >>> +{
> >>> +    struct syncobj_wait_entry *cur, *tmp;
> >>> +    struct dma_fence *prev;
> >>> +
> >>> +    dma_fence_get(fence);
> >>> +
> >>> +    spin_lock(&syncobj->lock);
> >>> +
> >>> +    prev = drm_syncobj_fence_get(syncobj);
> >>> +    /* You are adding an unorder point to timeline, which could
> >>> cause payload returned from query_ioctl is 0! */
> >>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
> >>
> >>
> >> I think the WARN/BUG macros should only fire when there is an issue
> >> with programming from within the kernel.
> >>
> >> But this particular warning can be triggered by an application.
> >>
> >>
> >> Probably best to just remove it?
> >
> > Yeah, that was also my argument against it.
> >
> > Key point here is that we still want to note somehow that userspace
> > did something wrong and returning an error is not an option.
> >
> > Maybe just use DRM_ERROR with a static variable to print the message
> > only once.
> >
> > Christian.
> 
> I don't really see any point in printing an error once. If you run your
> application twice you end up thinking there was an issue just on the first run
> but it's actually always wrong.
> 

Except this nitpick, is there any other concern to push whole patch set? Is that time to push whole patch set?

-David

> 
> Unless we're willing to take the syncobj lock for longer periods of time when
> adding points, I guess we'll have to defer validation to validation layers.
> 
> 
> -Lionel
> 
> >
> >>
> >>
> >> -Lionel
> >>
> >>
> >>> +    dma_fence_chain_init(chain, prev, fence, point);
> >>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
> >>> +
> >>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
> >>> +        list_del_init(&cur->node);
> >>> +        syncobj_wait_syncobj_func(syncobj, cur);
> >>> +    }
> >>> +    spin_unlock(&syncobj->lock);
> >>> +
> >>> +    /* Walk the chain once to trigger garbage collection */
> >>> +    dma_fence_chain_for_each(fence, prev);
> >>> +    dma_fence_put(prev);
> >>> +}
> >>> +EXPORT_SYMBOL(drm_syncobj_add_point);
> >>> +
> >>>   /**
> >>>    * drm_syncobj_replace_fence - replace fence in a sync object.
> >>>    * @syncobj: Sync object to replace fence in diff --git
> >>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
> >>> 0311c9fdbd2f..6cf7243a1dc5 100644
> >>> --- a/include/drm/drm_syncobj.h
> >>> +++ b/include/drm/drm_syncobj.h
> >>> @@ -27,6 +27,7 @@
> >>>   #define __DRM_SYNCOBJ_H__
> >>>     #include <linux/dma-fence.h>
> >>> +#include <linux/dma-fence-chain.h>
> >>>     struct drm_file;
> >>>   @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
> >>> *syncobj)
> >>>     struct drm_syncobj *drm_syncobj_find(struct drm_file
> >>> *file_private,
> >>>                        u32 handle);
> >>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> >>> +               struct dma_fence_chain *chain,
> >>> +               struct dma_fence *fence,
> >>> +               uint64_t point);
> >>>   void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
> >>>                      struct dma_fence *fence);
> >>>   int drm_syncobj_find_fence(struct drm_file *file_private,
> >>
> >>
> >
> >