[PATCH 02/11] dma-buf: add new dma_fence_chain container v2

[Zhou, David(ChunMing)]

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken at gmail.com>
> Sent: Monday, December 03, 2018 9:56 PM
> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; dri-devel at lists.freedesktop.org; amd-
> gfx at lists.freedesktop.org
> Subject: Re: [PATCH 02/11] dma-buf: add new dma_fence_chain container
> v2
> 
> Am 03.12.18 um 14:44 schrieb Chunming Zhou:
> >
> > 在 2018/12/3 21:28, Christian König 写道:
> >> Am 03.12.18 um 14:18 schrieb Chunming Zhou:
> >>> 在 2018/12/3 19:00, Christian König 写道:
> >>>> Am 03.12.18 um 06:25 schrieb zhoucm1:
> >>>>> On 2018年11月28日 22:50, Christian König wrote:
> >>>>>> Lockless container implementation similar to a dma_fence_array,
> >>>>>> but with only two elements per node and automatic garbage
> >>>>>> collection.
> >>>>>>
> >>>>>> v2: properly document dma_fence_chain_for_each, add
> >>>>>> dma_fence_chain_find_seqno,
> >>>>>>        drop prev reference during garbage collection if it's not
> >>>>>> a chain fence.
> >>>>>>
> >>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
> >>>>>> ---
 [snip]
> >>>>>> +
> >>>>>> +/**
> >>>>>> + * dma_fence_chain_init - initialize a fence chain
> >>>>>> + * @chain: the chain node to initialize
> >>>>>> + * @prev: the previous fence
> >>>>>> + * @fence: the current fence
> >>>>>> + *
> >>>>>> + * Initialize a new chain node and either start a new chain or
> >>>>>> +add
> >>>>>> the node to
> >>>>>> + * the existing chain of the previous fence.
> >>>>>> + */
> >>>>>> +void dma_fence_chain_init(struct dma_fence_chain *chain,
> >>>>>> +              struct dma_fence *prev,
> >>>>>> +              struct dma_fence *fence,
> >>>>>> +              uint64_t seqno)
> >>>>>> +{
> >>>>>> +    struct dma_fence_chain *prev_chain =
> >>>>>> +to_dma_fence_chain(prev);
> >>>>>> +    uint64_t context;
> >>>>>> +
> >>>>>> +    spin_lock_init(&chain->lock);
> >>>>>> +    chain->prev = prev;
> >>>>>> +    chain->fence = fence;
> >>>>>> +    chain->prev_seqno = 0;
> >>>>>> +    init_irq_work(&chain->work, dma_fence_chain_irq_work);
> >>>>>> +
> >>>>>> +    /* Try to reuse the context of the previous chain node. */
> >>>>>> +    if (prev_chain && seqno > prev->seqno &&
> >>>>>> +        __dma_fence_is_later(seqno, prev->seqno)) {
> >>>>> As your patch#1 makes __dma_fence_is_later only be valid for
> >>>>> 32bit, we cannot use it for 64bit here, we should remove it from
> >>>>> here, just compare seqno directly.
> >>>> That is intentional. We must make sure that the number both
> >>>> increments as 64bit number as well as not wraps around as 32bit
> number.
> >>>>
> >>>> In other words the largest difference between two sequence numbers
> >>>> userspace is allowed to submit is 1<<31.
> >>> Why? no one can make sure that, application users would only think
> >>> it is an uint64 sequence nubmer, and they can signal any advanced
> >>> point. I already see umd guys writing timeline test use max_uint64-1
> >>> as a final signal.
> >>> We shouldn't add this limitation here.
> >> We need to be backward compatible to hardware which can only do 32bit
> >> signaling with the dma-fence implementation.
> > I see that, you already explained that before.
> > but can't we just grep low 32bit of seqno only when 32bit hardware try
> > to use?
> >
> > then we can make dma_fence_later use 64bit comparation.
> 
> The problem is that we don't know at all times when to use a 32bit compare
> and when to use a 64bit compare.
> 
> What we could do is test if any of the upper 32bits of a sequence number is
> not 0 and if that is the case do a 64bit compare. This way max_uint64_t would
> still be handled correctly.
Sounds we can have a try, and we need mask upper 32bits for 32bit hardware case in the meanwhile,  right?

-David
> 
> 
> Christian.
> 
> >
> >> Otherwise dma_fence_later() could return an inconsistent result and
> >> break at other places.
> >>
> >> So if userspace wants to use more than 1<<31 difference between
> >> sequence numbers we need to push back on this.
> > It's rare case, but I don't think we can convince them add this
> > limitation. So we cannot add this limitation here.
> >
> > -David