On Thu, Jan 20, 2022 at 9:50 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> Am 19.01.22 um 18:16 schrieb Daniel Vetter:
> > On Wed, Jan 19, 2022 at 02:43:36PM +0100, Christian König wrote:
> >> Consolidate the wrapper functions to check for dma_fence
> >> subclasses in the dma_fence header.
> >>
> >> This makes it easier to document and also check the different
> >> requirements for fence containers in the subclasses.
> >>
> >> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> >> ---
> >> include/linux/dma-fence-array.h | 15 +------------
> >> include/linux/dma-fence-chain.h | 3 +--
> >> include/linux/dma-fence.h | 38 +++++++++++++++++++++++++++++++++
> >> 3 files changed, 40 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h
> >> index 303dd712220f..fec374f69e12 100644
> >> --- a/include/linux/dma-fence-array.h
> >> +++ b/include/linux/dma-fence-array.h
> >> @@ -45,19 +45,6 @@ struct dma_fence_array {
> >> struct irq_work work;
> >> };
> >>
> >> -extern const struct dma_fence_ops dma_fence_array_ops;
> >> -
> >> -/**
> >> - * dma_fence_is_array - check if a fence is from the array subsclass
> >> - * @fence: fence to test
> >> - *
> >> - * Return true if it is a dma_fence_array and false otherwise.
> >> - */
> >> -static inline bool dma_fence_is_array(struct dma_fence *fence)
> >> -{
> >> - return fence->ops == &dma_fence_array_ops;
> >> -}
> >> -
> >> /**
> >> * to_dma_fence_array - cast a fence to a dma_fence_array
> >> * @fence: fence to cast to a dma_fence_array
> >> @@ -68,7 +55,7 @@ static inline bool dma_fence_is_array(struct dma_fence *fence)
> >> static inline struct dma_fence_array *
> >> to_dma_fence_array(struct dma_fence *fence)
> >> {
> >> - if (fence->ops != &dma_fence_array_ops)
> >> + if (!fence || !dma_fence_is_array(fence))
> >> return NULL;
> >>
> >> return container_of(fence, struct dma_fence_array, base);
> >> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h
> >> index 54fe3443fd2c..ee906b659694 100644
> >> --- a/include/linux/dma-fence-chain.h
> >> +++ b/include/linux/dma-fence-chain.h
> >> @@ -49,7 +49,6 @@ struct dma_fence_chain {
> >> spinlock_t lock;
> >> };
> >>
> >> -extern const struct dma_fence_ops dma_fence_chain_ops;
> >>
> >> /**
> >> * to_dma_fence_chain - cast a fence to a dma_fence_chain
> >> @@ -61,7 +60,7 @@ extern const struct dma_fence_ops dma_fence_chain_ops;
> >> static inline struct dma_fence_chain *
> >> to_dma_fence_chain(struct dma_fence *fence)
> >> {
> >> - if (!fence || fence->ops != &dma_fence_chain_ops)
> >> + if (!fence || !dma_fence_is_chain(fence))
> >> return NULL;
> >>
> >> return container_of(fence, struct dma_fence_chain, base);
> >> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> >> index 1ea691753bd3..775cdc0b4f24 100644
> >> --- a/include/linux/dma-fence.h
> >> +++ b/include/linux/dma-fence.h
> >> @@ -587,4 +587,42 @@ struct dma_fence *dma_fence_get_stub(void);
> >> struct dma_fence *dma_fence_allocate_private_stub(void);
> >> u64 dma_fence_context_alloc(unsigned num);
> >>
> >> +extern const struct dma_fence_ops dma_fence_array_ops;
> >> +extern const struct dma_fence_ops dma_fence_chain_ops;
> >> +
> >> +/**
> >> + * dma_fence_is_array - check if a fence is from the array subclass
> >> + * @fence: the fence to test
> >> + *
> >> + * Return true if it is a dma_fence_array and false otherwise.
> >> + */
> >> +static inline bool dma_fence_is_array(struct dma_fence *fence)
> >> +{
> >> + return fence->ops == &dma_fence_array_ops;
> >> +}
> >> +
> >> +/**
> >> + * dma_fence_is_chain - check if a fence is from the chain subclass
> >> + * @fence: the fence to test
> >> + *
> >> + * Return true if it is a dma_fence_chain and false otherwise.
> >> + */
> >> +static inline bool dma_fence_is_chain(struct dma_fence *fence)
> >> +{
> >> + return fence->ops == &dma_fence_chain_ops;
> >> +}
> >> +
> >> +/**
> >> + * dma_fence_is_container - check if a fence is a container for other fences
> >> + * @fence: the fence to test
> >> + *
> >> + * Return true if this fence is a container for other fences, false otherwise.
> >> + * This is important since we can't build up large fence structure or otherwise
> >> + * we run into recursion during operation on those fences.
> >> + */
> >> +static inline bool dma_fence_is_container(struct dma_fence *fence)
> > Code looks all good, but I'm not super enthusiastic about exporting the
> > ops to drivers and letting them do random nonsense. At least i915 does
> > pretty enormous amounts of stuff with that instead of having pushed
> > priority boosting into dma-fence as a proper concept. And maybe a few
> > other things.
> >
> > Now i915-gem team having gone off the rails of good upstream conduct is
> > another thing maybe, but I'd like to not encourage that.
> >
> > So could we perhaps do this all in header which is entirely private to
> > drivers/dma-buf, like dma-fence-internal or so? And maybe whack a big
> > fixme onto the current abuse in drivers (of which __dma_fence_is_chain()
> > gets a special price for "not how upstream should be done" *sigh*).
>
> WTF is __dma_fence_is_chain? Seeing that for the first time now.
Yes :-/
> And yes even if you do priority boosting manually that code in i915 is
> just way to complicated.
>
> I'm sure you don't have any objections that I clean up that mess now you
> pointed it out :)
Go for it. I think ideally we could get rid of the dma_fence_chain_ops
export. Btw similar situation exists for dma_fence_is_array, so maybe
check those out too.
I think if we do want an interface for drivers then really the only
thing that should be accessible is a dma_fence_is_container and a
dma_fence_container_for_each_fence. Really no one's business to dig
into deeper details (at least once your dma_resv series has landed).
Thanks, Daniel
> Thanks,
> Christian.
>
> >
> > Cheers, Daniel
> >
> >> +{
> >> + return dma_fence_is_array(fence) || dma_fence_is_chain(fence);
> >> +}
> >> +
> >> #endif /* __LINUX_DMA_FENCE_H */
> >> --
> >> 2.25.1
> >>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
