-----Original Message----- From: Christian König ckoenig.leichtzumerken@gmail.com Sent: Monday, December 03, 2018 9:56 PM To: Zhou, David(ChunMing) David1.Zhou@amd.com; Koenig, Christian Christian.Koenig@amd.com; dri-devel@lists.freedesktop.org; amd- gfx@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@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