| Message ID | 20230311173513.1080397-2-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | drm/msm: Get rid of fence allocation in job_run() | expand |
Am 11.03.23 um 18:35 schrieb Rob Clark: > From: Rob Clark <robdclark@chromium.org> > > 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@chromium.org> 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. 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); >
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@chromium.org> >> >> 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@chromium.org> > > 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); >
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);