Re: [PATCH 1/2] dma-buf/dma-fence: Add dma_fence_init_noref()

[Christian König <christian.koenig@xxxxxxx>]

Am 13.03.23 um 08:13 schrieb Christian König:
> Am 11.03.23 um 18:35 schrieb Rob Clark:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > Add a way to initialize a fence without touching the refcount. This is
> > useful, for example, if the fence is embedded in a drm_sched_job.  In
> > this case the refcount will be initialized before the job is queued.
> > But the seqno of the hw_fence is not known until job_run().
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> Well that approach won't work. The fence can only be initialized in 
> the job_run() callback because only then the sequence number can be 
> determined.

Ah, wait a second! After reading the msm code I realized you are going 
to use this exactly the other way around that I think you use it.

In this case it would work, but that really needs better documentation. 
And I'm pretty sure it's not a good idea for msm, but let's discuss that 
on the other patch.

Regards,
Christian.

> Regards,
> Christian.
>
> > ---
> >   drivers/dma-buf/dma-fence.c | 43 ++++++++++++++++++++++++++++---------
> >   include/linux/dma-fence.h   |  2 ++
> >   2 files changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 74e36f6d05b0..97c05a465cb4 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -989,28 +989,27 @@ void dma_fence_describe(struct dma_fence 
> > *fence, struct seq_file *seq)
> >   EXPORT_SYMBOL(dma_fence_describe);
> >     /**
> > - * dma_fence_init - Initialize a custom fence.
> > + * dma_fence_init_noref - Initialize a custom fence without 
> > initializing refcount.
> >    * @fence: the fence to initialize
> >    * @ops: the dma_fence_ops for operations on this fence
> >    * @lock: the irqsafe spinlock to use for locking this fence
> >    * @context: the execution context this fence is run on
> >    * @seqno: a linear increasing sequence number for this context
> >    *
> > - * Initializes an allocated fence, the caller doesn't have to keep its
> > - * refcount after committing with this fence, but it will need to 
> > hold a
> > - * refcount again if &dma_fence_ops.enable_signaling gets called.
> > - *
> > - * context and seqno are used for easy comparison between fences, 
> > allowing
> > - * to check which fence is later by simply using dma_fence_later().
> > + * Like &dma_fence_init but does not initialize the refcount.  Suitable
> > + * for cases where the fence is embedded in another struct which has 
> > it's
> > + * refcount initialized before the fence is initialized.  Such as 
> > embedding
> > + * in a &drm_sched_job, where the job is created before knowing the 
> > seqno
> > + * of the hw_fence.
> >    */
> >   void
> > -dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops 
> > *ops,
> > -           spinlock_t *lock, u64 context, u64 seqno)
> > +dma_fence_init_noref(struct dma_fence *fence, const struct 
> > dma_fence_ops *ops,
> > +             spinlock_t *lock, u64 context, u64 seqno)
> >   {
> >       BUG_ON(!lock);
> >       BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
> > +    BUG_ON(!kref_read(&fence->refcount));
> >   -    kref_init(&fence->refcount);
> >       fence->ops = ops;
> >       INIT_LIST_HEAD(&fence->cb_list);
> >       fence->lock = lock;
> > @@ -1021,4 +1020,28 @@ dma_fence_init(struct dma_fence *fence, const 
> > struct dma_fence_ops *ops,
> >         trace_dma_fence_init(fence);
> >   }
> > +EXPORT_SYMBOL(dma_fence_init_noref);
> > +
> > +/**
> > + * dma_fence_init - Initialize a custom fence.
> > + * @fence: the fence to initialize
> > + * @ops: the dma_fence_ops for operations on this fence
> > + * @lock: the irqsafe spinlock to use for locking this fence
> > + * @context: the execution context this fence is run on
> > + * @seqno: a linear increasing sequence number for this context
> > + *
> > + * Initializes an allocated fence, the caller doesn't have to keep its
> > + * refcount after committing with this fence, but it will need to 
> > hold a
> > + * refcount again if &dma_fence_ops.enable_signaling gets called.
> > + *
> > + * context and seqno are used for easy comparison between fences, 
> > allowing
> > + * to check which fence is later by simply using dma_fence_later().
> > + */
> > +void
> > +dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops 
> > *ops,
> > +           spinlock_t *lock, u64 context, u64 seqno)
> > +{
> > +    kref_init(&fence->refcount);
> > +    dma_fence_init_noref(fence, ops, lock, context, seqno);
> > +}
> >   EXPORT_SYMBOL(dma_fence_init);
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index d54b595a0fe0..f617c78a2e0a 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -279,6 +279,8 @@ struct dma_fence_ops {
> >       void (*set_deadline)(struct dma_fence *fence, ktime_t deadline);
> >   };
> >   +void dma_fence_init_noref(struct dma_fence *fence, const struct 
> > dma_fence_ops *ops,
> > +              spinlock_t *lock, u64 context, u64 seqno);
> >   void dma_fence_init(struct dma_fence *fence, const struct 
> > dma_fence_ops *ops,
> >               spinlock_t *lock, u64 context, u64 seqno);