- Linaro-mm-sig - lists.linaro.org

Re: [Linaro-mm-sig] [PATCH net-next 0/5] brcm ASP 2.0 Ethernet controller

by Andrew Lunn

On Fri, Sep 24, 2021 at 02:44:46PM -0700, Justin Chen wrote: > This patch set adds support for Broadcom's ASP 2.0 Ethernet controller. Hi Justin Does the hardware support L2 switching between the two ports? I'm just wondering if later this is going to be modified into a switchdev driver? Andrew

3 years, 8 months

1
0
0 0

[PATCH 01/27] dma-buf: add dma_resv_for_each_fence_unlocked v6

by Christian König

Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-resv.c | 98 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 95 ++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..97af397304f3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,104 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); + } while (cursor->fence && dma_fence_is_signaled(cursor->fence)); +} + +/** + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the first fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) +{ + rcu_read_lock(); + do { + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); + +/** + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the next fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) +{ + bool restart; + + rcu_read_lock(); + cursor->is_restarted = false; + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); + do { + if (restart) + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + restart = true; + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..5d7d28cb9008 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,101 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_iter - current position into the dma_resv fences + * + * Don't touch this directly in the driver, use the accessor function instead. + */ +struct dma_resv_iter { + /** @obj: The dma_resv object we iterate over */ + struct dma_resv *obj; + + /** @all_fences: If all fences should be returned */ + bool all_fences; + + /** @fence: the currently handled fence */ + struct dma_fence *fence; + + /** @seq: sequence number to check for modifications */ + unsigned int seq; + + /** @index: index into the shared fences */ + unsigned int index; + + /** @fences: the shared fences */ + struct dma_resv_list *fences; + + /** @is_restarted: true if this is the first returned fence */ + bool is_restarted; +}; + +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); + +/** + * dma_resv_iter_begin - initialize a dma_resv_iter object + * @cursor: The dma_resv_iter object to initialize + * @obj: The dma_resv object which we want to iterate over + * @all_fences: If all fences should be returned or just the exclusive one + */ +static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor, + struct dma_resv *obj, + bool all_fences) +{ + cursor->obj = obj; + cursor->all_fences = all_fences; + cursor->fence = NULL; +} + +/** + * dma_resv_iter_end - cleanup a dma_resv_iter object + * @cursor: the dma_resv_iter object which should be cleaned up + * + * Make sure that the reference to the fence in the cursor is properly + * dropped. + */ +static inline void dma_resv_iter_end(struct dma_resv_iter *cursor) +{ + dma_fence_put(cursor->fence); +} + +/** + * dma_resv_iter_is_exclusive - test if the current fence is the exclusive one + * @cursor: the cursor of the current position + * + * Returns true if the currently returned fence is the exclusive one. + */ +static inline bool dma_resv_iter_is_exclusive(struct dma_resv_iter *cursor) +{ + return cursor->index == -1; +} + +/** + * dma_resv_iter_is_restarted - test if this is the first fence after a restart + * @cursor: the cursor with the current position + * + * Return true if this is the first fence in an iteration after a restart. + */ +static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) +{ + return cursor->is_restarted; +} + +/** + * dma_resv_for_each_fence_unlocked - unlocked fence iterator + * @cursor: a struct dma_resv_iter pointer + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * &dma_resv.lock and using RCU instead. The cursor needs to be initialized + * with dma_resv_iter_begin() and cleaned up with dma_resv_iter_end(). Inside + * the iterator a reference to the dma_fence is held and the RCU lock dropped. + * When the dma_resv is modified the iteration starts over again. + */ +#define dma_resv_for_each_fence_unlocked(cursor, fence) \ + for (fence = dma_resv_iter_first_unlocked(cursor); \ + fence; fence = dma_resv_iter_next_unlocked(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -- 2.25.1

3 years, 8 months

1
26
0 0

Re: [Linaro-mm-sig] [RFC PATCH 2/4] DRM: Add support of AI Processor Unit (APU)

by Christian König

Am 23.09.21 um 02:58 schrieb Dave Airlie: > On Sat, 18 Sept 2021 at 07:57, Alexandre Bailon <abailon(a)baylibre.com> wrote: >> Some Mediatek SoC provides hardware accelerator for AI / ML. >> This driver provides the infrastructure to manage memory >> shared between host CPU and the accelerator, and to submit >> jobs to the accelerator. >> The APU itself is managed by remoteproc so this drivers >> relies on remoteproc to found the APU and get some important data >> from it. But, the driver is quite generic and it should possible >> to manage accelerator using another ways. >> This driver doesn't manage itself the data transmitions. >> It must be registered by another driver implementing the transmitions. >> >> Signed-off-by: Alexandre Bailon <abailon(a)baylibre.com> >> [SNIP] >> Please refer to >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.kerne… >> >> here and below in many places. >> >> There's a lot of missing padding/alignment here. There is also the pahole utility which show you nicely where you need padding for your IOCTL structures. For example "pahole drivers/gpu/drm/amd/amdgpu/amdgpu.ko -C drm_amdgpu_gem_va" gives you: struct drm_amdgpu_gem_va { __u32 handle; /* 0 4 */ __u32 _pad; /* 4 4 */ __u32 operation; /* 8 4 */ __u32 flags; /* 12 4 */ __u64 va_address; /* 16 8 */ __u64 offset_in_bo; /* 24 8 */ __u64 map_size; /* 32 8 */ /* size: 40, cachelines: 1, members: 7 */ /* last cacheline: 40 bytes */ }; And as you can see we have added the _pad field to our IOCTL parameter structure to properly align the 64bit members. Regards, Christian. >> >> I'm trying to find the time to review this stack in full, any writeups >> on how this is used from userspace would be useful (not just the code >> repo, but some sort of how do I get at it) it reads as kinda generic >> (calling it apu), but then has some specifics around device binding. >> >> Dave.

3 years, 8 months

1
0
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi guys, The version I've send out yesterday had a rather obvious coding error and I honestly forgot the cover letter. This one here is better tested and will now hopefully not be torn apart from the CI system immediately. I tried to address all review and documentation comments as best as I could, so I'm hoping that we can now considering pushing this. Cheers, Christian.

3 years, 8 months

2
40
0 0

Re: [Linaro-mm-sig] [PATCH v3 4/9] drm/scheduler: Add fence deadline support

by Rob Clark

On Wed, Sep 22, 2021 at 7:31 AM Andrey Grodzovsky <andrey.grodzovsky(a)amd.com> wrote: > > > On 2021-09-21 11:32 p.m., Rob Clark wrote: > > On Tue, Sep 21, 2021 at 7:18 PM Andrey Grodzovsky > > <andrey.grodzovsky(a)amd.com> wrote: > >> > >> On 2021-09-21 4:47 p.m., Rob Clark wrote: > >>> On Tue, Sep 21, 2021 at 1:09 PM Andrey Grodzovsky > >>> <andrey.grodzovsky(a)amd.com> wrote: > >>>> On 2021-09-03 2:47 p.m., Rob Clark wrote: > >>>> > >>>>> From: Rob Clark <robdclark(a)chromium.org> > >>>>> > >>>>> As the finished fence is the one that is exposed to userspace, and > >>>>> therefore the one that other operations, like atomic update, would > >>>>> block on, we need to propagate the deadline from from the finished > >>>>> fence to the actual hw fence. > >>>>> > >>>>> v2: Split into drm_sched_fence_set_parent() (ckoenig) > >>>>> > >>>>> Signed-off-by: Rob Clark <robdclark(a)chromium.org> > >>>>> --- > >>>>> drivers/gpu/drm/scheduler/sched_fence.c | 34 +++++++++++++++++++++++++ > >>>>> drivers/gpu/drm/scheduler/sched_main.c | 2 +- > >>>>> include/drm/gpu_scheduler.h | 8 ++++++ > >>>>> 3 files changed, 43 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > >>>>> index bcea035cf4c6..4fc41a71d1c7 100644 > >>>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c > >>>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c > >>>>> @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > >>>>> dma_fence_put(&fence->scheduled); > >>>>> } > >>>>> > >>>>> +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > >>>>> + ktime_t deadline) > >>>>> +{ > >>>>> + struct drm_sched_fence *fence = to_drm_sched_fence(f); > >>>>> + unsigned long flags; > >>>>> + > >>>>> + spin_lock_irqsave(&fence->lock, flags); > >>>>> + > >>>>> + /* If we already have an earlier deadline, keep it: */ > >>>>> + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > >>>>> + ktime_before(fence->deadline, deadline)) { > >>>>> + spin_unlock_irqrestore(&fence->lock, flags); > >>>>> + return; > >>>>> + } > >>>>> + > >>>>> + fence->deadline = deadline; > >>>>> + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > >>>>> + > >>>>> + spin_unlock_irqrestore(&fence->lock, flags); > >>>>> + > >>>>> + if (fence->parent) > >>>>> + dma_fence_set_deadline(fence->parent, deadline); > >>>>> +} > >>>>> + > >>>>> static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > >>>>> .get_driver_name = drm_sched_fence_get_driver_name, > >>>>> .get_timeline_name = drm_sched_fence_get_timeline_name, > >>>>> @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > >>>>> .get_driver_name = drm_sched_fence_get_driver_name, > >>>>> .get_timeline_name = drm_sched_fence_get_timeline_name, > >>>>> .release = drm_sched_fence_release_finished, > >>>>> + .set_deadline = drm_sched_fence_set_deadline_finished, > >>>>> }; > >>>>> > >>>>> struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > >>>>> @@ -152,6 +177,15 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > >>>>> } > >>>>> EXPORT_SYMBOL(to_drm_sched_fence); > >>>>> > >>>>> +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > >>>>> + struct dma_fence *fence) > >>>>> +{ > >>>>> + s_fence->parent = dma_fence_get(fence); > >>>>> + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > >>>>> + &s_fence->finished.flags)) > >>>>> + dma_fence_set_deadline(fence, s_fence->deadline); > >>>> I believe above you should pass be s_fence->finished to > >>>> dma_fence_set_deadline > >>>> instead it fence which is the HW fence itself. > >>> Hmm, unless this has changed recently with some patches I don't have, > >>> s_fence->parent is the one signalled by hw, so it is the one we want > >>> to set the deadline on > >>> > >>> BR, > >>> -R > >> > >> No it didn't change. But then when exactly will > >> drm_sched_fence_set_deadline_finished > >> execute such that fence->parent != NULL ? In other words, I am not clear > >> how propagation > >> happens otherwise - if dma_fence_set_deadline is called with the HW > >> fence then the assumption > >> here is that driver provided driver specific > >> dma_fence_ops.dma_fence_set_deadline callback executes > >> but I was under impression that drm_sched_fence_set_deadline_finished is > >> the one that propagates > >> the deadline to the HW fence's callback and for it to execute > >> dma_fence_set_deadline needs to be called > >> with s_fence->finished. > > Assuming I didn't screw up drm/msm conversion to scheduler, > > &s_fence->finished is the one that will be returned to userspace.. and > > later passed back to kernel for atomic commit (or to the compositor). > > So it is the one that fence->set_deadline() will be called on. But > > s_fence->parent is the actual hw fence that needs to know about the > > deadline. Depending on whether or not the job has been written into > > hw ringbuffer or not, there are two cases: > > > > 1) not scheduled yet, s_fence will store the deadline and propagate it > > later once s_fence->parent is known > > > And by later you mean the call to drm_sched_fence_set_parent > after HW fence is returned ? If yes I think i get it now. Yup :-) BR, -R > Andrey > > > > 2) already scheduled, in which case s_fence->finished.set_deadline > > will propagate it directly to the real fence > > > > BR, > > -R > > > >> Andrey > >> > >> > >> > >>>> Andrey > >>>> > >>>> > >>>>> +} > >>>>> + > >>>>> struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, > >>>>> void *owner) > >>>>> { > >>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > >>>>> index 595e47ff7d06..27bf0ac0625f 100644 > >>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c > >>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c > >>>>> @@ -978,7 +978,7 @@ static int drm_sched_main(void *param) > >>>>> drm_sched_fence_scheduled(s_fence); > >>>>> > >>>>> if (!IS_ERR_OR_NULL(fence)) { > >>>>> - s_fence->parent = dma_fence_get(fence); > >>>>> + drm_sched_fence_set_parent(s_fence, fence); > >>>>> r = dma_fence_add_callback(fence, &sched_job->cb, > >>>>> drm_sched_job_done_cb); > >>>>> if (r == -ENOENT) > >>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > >>>>> index 7f77a455722c..158ddd662469 100644 > >>>>> --- a/include/drm/gpu_scheduler.h > >>>>> +++ b/include/drm/gpu_scheduler.h > >>>>> @@ -238,6 +238,12 @@ struct drm_sched_fence { > >>>>> */ > >>>>> struct dma_fence finished; > >>>>> > >>>>> + /** > >>>>> + * @deadline: deadline set on &drm_sched_fence.finished which > >>>>> + * potentially needs to be propagated to &drm_sched_fence.parent > >>>>> + */ > >>>>> + ktime_t deadline; > >>>>> + > >>>>> /** > >>>>> * @parent: the fence returned by &drm_sched_backend_ops.run_job > >>>>> * when scheduling the job on hardware. We signal the > >>>>> @@ -505,6 +511,8 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, > >>>>> enum drm_sched_priority priority); > >>>>> bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); > >>>>> > >>>>> +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > >>>>> + struct dma_fence *fence); > >>>>> struct drm_sched_fence *drm_sched_fence_alloc( > >>>>> struct drm_sched_entity *s_entity, void *owner); > >>>>> void drm_sched_fence_init(struct drm_sched_fence *fence,

3 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 4/9] drm/scheduler: Add fence deadline support

by Rob Clark

On Tue, Sep 21, 2021 at 7:18 PM Andrey Grodzovsky <andrey.grodzovsky(a)amd.com> wrote: > > > On 2021-09-21 4:47 p.m., Rob Clark wrote: > > On Tue, Sep 21, 2021 at 1:09 PM Andrey Grodzovsky > > <andrey.grodzovsky(a)amd.com> wrote: > >> On 2021-09-03 2:47 p.m., Rob Clark wrote: > >> > >>> From: Rob Clark <robdclark(a)chromium.org> > >>> > >>> As the finished fence is the one that is exposed to userspace, and > >>> therefore the one that other operations, like atomic update, would > >>> block on, we need to propagate the deadline from from the finished > >>> fence to the actual hw fence. > >>> > >>> v2: Split into drm_sched_fence_set_parent() (ckoenig) > >>> > >>> Signed-off-by: Rob Clark <robdclark(a)chromium.org> > >>> --- > >>> drivers/gpu/drm/scheduler/sched_fence.c | 34 +++++++++++++++++++++++++ > >>> drivers/gpu/drm/scheduler/sched_main.c | 2 +- > >>> include/drm/gpu_scheduler.h | 8 ++++++ > >>> 3 files changed, 43 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > >>> index bcea035cf4c6..4fc41a71d1c7 100644 > >>> --- a/drivers/gpu/drm/scheduler/sched_fence.c > >>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c > >>> @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > >>> dma_fence_put(&fence->scheduled); > >>> } > >>> > >>> +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > >>> + ktime_t deadline) > >>> +{ > >>> + struct drm_sched_fence *fence = to_drm_sched_fence(f); > >>> + unsigned long flags; > >>> + > >>> + spin_lock_irqsave(&fence->lock, flags); > >>> + > >>> + /* If we already have an earlier deadline, keep it: */ > >>> + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > >>> + ktime_before(fence->deadline, deadline)) { > >>> + spin_unlock_irqrestore(&fence->lock, flags); > >>> + return; > >>> + } > >>> + > >>> + fence->deadline = deadline; > >>> + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > >>> + > >>> + spin_unlock_irqrestore(&fence->lock, flags); > >>> + > >>> + if (fence->parent) > >>> + dma_fence_set_deadline(fence->parent, deadline); > >>> +} > >>> + > >>> static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > >>> .get_driver_name = drm_sched_fence_get_driver_name, > >>> .get_timeline_name = drm_sched_fence_get_timeline_name, > >>> @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > >>> .get_driver_name = drm_sched_fence_get_driver_name, > >>> .get_timeline_name = drm_sched_fence_get_timeline_name, > >>> .release = drm_sched_fence_release_finished, > >>> + .set_deadline = drm_sched_fence_set_deadline_finished, > >>> }; > >>> > >>> struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > >>> @@ -152,6 +177,15 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > >>> } > >>> EXPORT_SYMBOL(to_drm_sched_fence); > >>> > >>> +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > >>> + struct dma_fence *fence) > >>> +{ > >>> + s_fence->parent = dma_fence_get(fence); > >>> + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > >>> + &s_fence->finished.flags)) > >>> + dma_fence_set_deadline(fence, s_fence->deadline); > >> > >> I believe above you should pass be s_fence->finished to > >> dma_fence_set_deadline > >> instead it fence which is the HW fence itself. > > Hmm, unless this has changed recently with some patches I don't have, > > s_fence->parent is the one signalled by hw, so it is the one we want > > to set the deadline on > > > > BR, > > -R > > > No it didn't change. But then when exactly will > drm_sched_fence_set_deadline_finished > execute such that fence->parent != NULL ? In other words, I am not clear > how propagation > happens otherwise - if dma_fence_set_deadline is called with the HW > fence then the assumption > here is that driver provided driver specific > dma_fence_ops.dma_fence_set_deadline callback executes > but I was under impression that drm_sched_fence_set_deadline_finished is > the one that propagates > the deadline to the HW fence's callback and for it to execute > dma_fence_set_deadline needs to be called > with s_fence->finished. Assuming I didn't screw up drm/msm conversion to scheduler, &s_fence->finished is the one that will be returned to userspace.. and later passed back to kernel for atomic commit (or to the compositor). So it is the one that fence->set_deadline() will be called on. But s_fence->parent is the actual hw fence that needs to know about the deadline. Depending on whether or not the job has been written into hw ringbuffer or not, there are two cases: 1) not scheduled yet, s_fence will store the deadline and propagate it later once s_fence->parent is known 2) already scheduled, in which case s_fence->finished.set_deadline will propagate it directly to the real fence BR, -R > Andrey > > > > > > >> Andrey > >> > >> > >>> +} > >>> + > >>> struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, > >>> void *owner) > >>> { > >>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > >>> index 595e47ff7d06..27bf0ac0625f 100644 > >>> --- a/drivers/gpu/drm/scheduler/sched_main.c > >>> +++ b/drivers/gpu/drm/scheduler/sched_main.c > >>> @@ -978,7 +978,7 @@ static int drm_sched_main(void *param) > >>> drm_sched_fence_scheduled(s_fence); > >>> > >>> if (!IS_ERR_OR_NULL(fence)) { > >>> - s_fence->parent = dma_fence_get(fence); > >>> + drm_sched_fence_set_parent(s_fence, fence); > >>> r = dma_fence_add_callback(fence, &sched_job->cb, > >>> drm_sched_job_done_cb); > >>> if (r == -ENOENT) > >>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > >>> index 7f77a455722c..158ddd662469 100644 > >>> --- a/include/drm/gpu_scheduler.h > >>> +++ b/include/drm/gpu_scheduler.h > >>> @@ -238,6 +238,12 @@ struct drm_sched_fence { > >>> */ > >>> struct dma_fence finished; > >>> > >>> + /** > >>> + * @deadline: deadline set on &drm_sched_fence.finished which > >>> + * potentially needs to be propagated to &drm_sched_fence.parent > >>> + */ > >>> + ktime_t deadline; > >>> + > >>> /** > >>> * @parent: the fence returned by &drm_sched_backend_ops.run_job > >>> * when scheduling the job on hardware. We signal the > >>> @@ -505,6 +511,8 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, > >>> enum drm_sched_priority priority); > >>> bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); > >>> > >>> +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > >>> + struct dma_fence *fence); > >>> struct drm_sched_fence *drm_sched_fence_alloc( > >>> struct drm_sched_entity *s_entity, void *owner); > >>> void drm_sched_fence_init(struct drm_sched_fence *fence,

3 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 4/9] drm/scheduler: Add fence deadline support

by Rob Clark

On Tue, Sep 21, 2021 at 1:09 PM Andrey Grodzovsky <andrey.grodzovsky(a)amd.com> wrote: > > On 2021-09-03 2:47 p.m., Rob Clark wrote: > > > From: Rob Clark <robdclark(a)chromium.org> > > > > As the finished fence is the one that is exposed to userspace, and > > therefore the one that other operations, like atomic update, would > > block on, we need to propagate the deadline from from the finished > > fence to the actual hw fence. > > > > v2: Split into drm_sched_fence_set_parent() (ckoenig) > > > > Signed-off-by: Rob Clark <robdclark(a)chromium.org> > > --- > > drivers/gpu/drm/scheduler/sched_fence.c | 34 +++++++++++++++++++++++++ > > drivers/gpu/drm/scheduler/sched_main.c | 2 +- > > include/drm/gpu_scheduler.h | 8 ++++++ > > 3 files changed, 43 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c > > index bcea035cf4c6..4fc41a71d1c7 100644 > > --- a/drivers/gpu/drm/scheduler/sched_fence.c > > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > > @@ -128,6 +128,30 @@ static void drm_sched_fence_release_finished(struct dma_fence *f) > > dma_fence_put(&fence->scheduled); > > } > > > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > > + ktime_t deadline) > > +{ > > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&fence->lock, flags); > > + > > + /* If we already have an earlier deadline, keep it: */ > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > > + ktime_before(fence->deadline, deadline)) { > > + spin_unlock_irqrestore(&fence->lock, flags); > > + return; > > + } > > + > > + fence->deadline = deadline; > > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > > + > > + spin_unlock_irqrestore(&fence->lock, flags); > > + > > + if (fence->parent) > > + dma_fence_set_deadline(fence->parent, deadline); > > +} > > + > > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > > .get_driver_name = drm_sched_fence_get_driver_name, > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > @@ -138,6 +162,7 @@ static const struct dma_fence_ops drm_sched_fence_ops_finished = { > > .get_driver_name = drm_sched_fence_get_driver_name, > > .get_timeline_name = drm_sched_fence_get_timeline_name, > > .release = drm_sched_fence_release_finished, > > + .set_deadline = drm_sched_fence_set_deadline_finished, > > }; > > > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > > @@ -152,6 +177,15 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > > } > > EXPORT_SYMBOL(to_drm_sched_fence); > > > > +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > > + struct dma_fence *fence) > > +{ > > + s_fence->parent = dma_fence_get(fence); > > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > > + &s_fence->finished.flags)) > > + dma_fence_set_deadline(fence, s_fence->deadline); > > > I believe above you should pass be s_fence->finished to > dma_fence_set_deadline > instead it fence which is the HW fence itself. Hmm, unless this has changed recently with some patches I don't have, s_fence->parent is the one signalled by hw, so it is the one we want to set the deadline on BR, -R > Andrey > > > > +} > > + > > struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, > > void *owner) > > { > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c > > index 595e47ff7d06..27bf0ac0625f 100644 > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > @@ -978,7 +978,7 @@ static int drm_sched_main(void *param) > > drm_sched_fence_scheduled(s_fence); > > > > if (!IS_ERR_OR_NULL(fence)) { > > - s_fence->parent = dma_fence_get(fence); > > + drm_sched_fence_set_parent(s_fence, fence); > > r = dma_fence_add_callback(fence, &sched_job->cb, > > drm_sched_job_done_cb); > > if (r == -ENOENT) > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > > index 7f77a455722c..158ddd662469 100644 > > --- a/include/drm/gpu_scheduler.h > > +++ b/include/drm/gpu_scheduler.h > > @@ -238,6 +238,12 @@ struct drm_sched_fence { > > */ > > struct dma_fence finished; > > > > + /** > > + * @deadline: deadline set on &drm_sched_fence.finished which > > + * potentially needs to be propagated to &drm_sched_fence.parent > > + */ > > + ktime_t deadline; > > + > > /** > > * @parent: the fence returned by &drm_sched_backend_ops.run_job > > * when scheduling the job on hardware. We signal the > > @@ -505,6 +511,8 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, > > enum drm_sched_priority priority); > > bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); > > > > +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > > + struct dma_fence *fence); > > struct drm_sched_fence *drm_sched_fence_alloc( > > struct drm_sched_entity *s_entity, void *owner); > > void drm_sched_fence_init(struct drm_sched_fence *fence,

3 years, 8 months

1
0
0 0

[PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v3

by Christian König

Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-resv.c | 98 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 95 ++++++++++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..11b5399f4bd3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,104 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) { + rcu_read_lock(); + cursor->fences = dma_resv_shared_list(cursor->obj); + rcu_read_unlock(); + } else { + cursor->fences = NULL; + } + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iterration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index++ == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->fence = dma_fence_get_rcu(cursor->fence); + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index; + + cursor->fence = rcu_dereference(fences->shared[idx]); + cursor->fence = dma_fence_get_rcu(cursor->fence); + } + } while (cursor->fence && dma_fence_is_signaled(cursor->fence)); +} + +/** + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the first fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) +{ + rcu_read_lock(); + do { + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); + +/** + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the next fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) +{ + bool restart; + + rcu_read_lock(); + cursor->is_restarted = false; + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); + do { + if (restart) + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + restart = true; + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..baf77a542392 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,101 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_iter - current position into the dma_resv fences + * + * Don't touch this directly in the driver, use the accessor function instead. + */ +struct dma_resv_iter { + /** @obj: The dma_resv object we iterate over */ + struct dma_resv *obj; + + /** @all_fences: If all fences should be returned */ + bool all_fences; + + /** @fence: the currently handled fence */ + struct dma_fence *fence; + + /** @seq: sequence number to check for modifications */ + unsigned int seq; + + /** @index: index into the shared fences */ + unsigned int index; + + /** @fences: the shared fences */ + struct dma_resv_list *fences; + + /** @is_restarted: true if this is the first returned fence */ + bool is_restarted; +}; + +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); + +/** + * dma_resv_iter_begin - initialize a dma_resv_iter object + * @cursor: The dma_resv_iter object to initialize + * @obj: The dma_resv object which we want to iterator over + * @all_fences: If all fences should be returned or just the exclusive one + */ +static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor, + struct dma_resv *obj, + bool all_fences) +{ + cursor->obj = obj; + cursor->all_fences = all_fences; + cursor->fence = NULL; +} + +/** + * dma_resv_iter_end - cleanup a dma_resv_iter object + * @cursor: the dma_resv_iter object which should be cleaned up + * + * Make sure that the reference to the fence in the cursor is properly + * dropped. + */ +static inline void dma_resv_iter_end(struct dma_resv_iter *cursor) +{ + dma_fence_put(cursor->fence); +} + +/** + * dma_resv_iter_is_exclusive - test if the current fence is the exclusive one + * @cursor: the cursor of the current position + * + * Returns true if the currently returned fence is the exclusive one. + */ +static inline bool dma_resv_iter_is_exclusive(struct dma_resv_iter *cursor) +{ + return cursor->index == -1; +} + +/** + * dma_resv_iter_is_restarted - test if this is the first fence after a restart + * @cursor: the cursor with the current position + * + * Return true if this is the first fence in an interation after a restart. + */ +static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) +{ + return cursor->is_restarted; +} + +/** + * dma_resv_for_each_fence_unlocked - unlocked fence iterator + * @cursor: a struct dma_resv_iter pointer + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * &dma_resv.lock and using RCU instead. The cursor needs to be initialized + * with dma_resv_iter_begin() and cleaned up with dma_resv_iter_end(). Inside + * the iterator a reference to the dma_fence is hold and the RCU lock dropped. + * When the dma_resv is modified the iteration starts over again. + */ +#define dma_resv_for_each_fence_unlocked(cursor, fence) \ + for (fence = dma_resv_iter_first_unlocked(cursor); \ + fence; fence = dma_resv_iter_next_unlocked(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) -- 2.25.1

3 years, 8 months

1
25
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hopefully the last round for this. Added dma_resv_iter_begin/end as requested by Daniel. Fixed a bunch of problems pointed out by the CI systems and found a few more myselve. Please review and/or comment, Christian.

3 years, 8 months

3
55
0 0

Re: [Linaro-mm-sig] [RFC PATCH 1/4] dt-bindings: Add bidings for mtk, apu-drm

by Rob Herring

On Fri, Sep 17, 2021 at 02:59:42PM +0200, Alexandre Bailon wrote: > This adds the device tree bindings for the APU DRM driver. > > Signed-off-by: Alexandre Bailon <abailon(a)baylibre.com> > --- > .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > > diff --git a/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > new file mode 100644 > index 0000000000000..6f432d3ea478c > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > @@ -0,0 +1,38 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/gpu/mediatek,apu-drm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: AI Processor Unit DRM DRM is a linux thing, not h/w. > + > +properties: > + compatible: > + const: mediatek,apu-drm > + > + remoteproc: So is remoteproc. Why don't you have the remoteproc driver create the DRM device? > + maxItems: 2 > + description: > + Handle to remoteproc devices controlling the APU > + > + iova: > + maxItems: 1 > + description: > + Address and size of virtual memory that could used by the APU Why does this need to be in DT? If you need to reserve certain VAs, then this discussion[1] might be of interest. Rob [1] https://lore.kernel.org/all/YUIPCxnyRutMS47%2F@orome.fritz.box/

3 years, 8 months

1
0
0 0

[PATCH] panfrost: make mediatek_mt8183_supplies and mediatek_mt8183_pm_domains static

by Jiapeng Chong

This symbol is not used outside of panfrost_drv.c, so marks it static. Fix the following sparse warning: drivers/gpu/drm/panfrost/panfrost_drv.c:641:12: warning: symbol 'mediatek_mt8183_supplies' was not declared. Should it be static? drivers/gpu/drm/panfrost/panfrost_drv.c:642:12: warning: symbol 'mediatek_mt8183_pm_domains' was not declared. Should it be static? Reported-by: Abaci Robot <abaci(a)linux.alibaba.com> Signed-off-by: Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 077cbbf..82ad9a6 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -638,8 +638,8 @@ static int panfrost_remove(struct platform_device *pdev) .vendor_quirk = panfrost_gpu_amlogic_quirk, }; -const char * const mediatek_mt8183_supplies[] = { "mali", "sram" }; -const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; +static const char * const mediatek_mt8183_supplies[] = { "mali", "sram" }; +static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; static const struct panfrost_compatible mediatek_mt8183_data = { .num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies), .supply_names = mediatek_mt8183_supplies, -- 1.8.3.1

3 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 1/4] dt-bindings: Add bidings for mtk, apu-drm

by Rob Herring

On Fri, 17 Sep 2021 14:59:42 +0200, Alexandre Bailon wrote: > This adds the device tree bindings for the APU DRM driver. > > Signed-off-by: Alexandre Bailon <abailon(a)baylibre.com> > --- > .../devicetree/bindings/gpu/mtk,apu-drm.yaml | 38 +++++++++++++++++++ > 1 file changed, 38 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml: 'maintainers' is a required property hint: Metaschema for devicetree binding documentation from schema $id: http://devicetree.org/meta-schemas/base.yaml# ./Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/gpu/mtk,apu-drm.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml: ignoring, error in schema: warning: no schema found in file: ./Documentation/devicetree/bindings/gpu/mtk,apu-drm.yaml Documentation/devicetree/bindings/gpu/mtk,apu-drm.example.dts:19.15-23.11: Warning (unit_address_vs_reg): /example-0/apu@0: node has a unit name, but no reg or ranges property Documentation/devicetree/bindings/gpu/mtk,apu-drm.example.dt.yaml:0:0: /example-0/apu@0: failed to match any schema with compatible: ['mediatek,apu-drm'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1529388 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.

3 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v6 0/2] Add p2p via dmabuf to habanalabs

by Daniel Vetter

On Wed, Sep 15, 2021 at 10:45:36AM +0300, Oded Gabbay wrote: > On Tue, Sep 14, 2021 at 7:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Tue, Sep 14, 2021 at 04:18:31PM +0200, Daniel Vetter wrote: > > > On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > > > > Hi, > > > > Re-sending this patch-set following the release of our user-space TPC > > > > compiler and runtime library. > > > > > > > > I would appreciate a review on this. > > > > > > I think the big open we have is the entire revoke discussions. Having the > > > option to let dma-buf hang around which map to random local memory ranges, > > > without clear ownership link and a way to kill it sounds bad to me. > > > > > > I think there's a few options: > > > - We require revoke support. But I've heard rdma really doesn't like that, > > > I guess because taking out an MR while holding the dma_resv_lock would > > > be an inversion, so can't be done. Jason, can you recap what exactly the > > > hold-up was again that makes this a no-go? > > > > RDMA HW can't do revoke. Like why? I'm assuming when the final open handle or whatever for that MR is closed, you do clean up everything? Or does that MR still stick around forever too? > > So we have to exclude almost all the HW and several interesting use > > cases to enable a revoke operation. > > > > > - For non-revokable things like these dma-buf we'd keep a drm_master > > > reference around. This would prevent the next open to acquire > > > ownership rights, which at least prevents all the nasty potential > > > problems. > > > > This is what I generally would expect, the DMABUF FD and its DMA > > memory just floats about until the unrevokable user releases it, which > > happens when the FD that is driving the import eventually gets closed. > This is exactly what we are doing in the driver. We make sure > everything is valid until the unrevokable user releases it and that > happens only when the dmabuf fd gets closed. > And the user can't close it's fd of the device until he performs the > above, so there is no leakage between users. Maybe I got the device security model all wrong, but I thought Guadi is single user, and the only thing it protects is the system against the Gaudi device trhough iommu/device gart. So roughly the following can happen: 1. User A opens gaudi device, sets up dma-buf export 2. User A registers that with RDMA, or anything else that doesn't support revoke. 3. User A closes gaudi device 4. User B opens gaudi device, assumes that it has full control over the device and uploads some secrets, which happen to end up in the dma-buf region user A set up 5. User B extracts secrets. > > I still don't think any of the complexity is needed, pinnable memory > > is a thing in Linux, just account for it in mlocked and that is > > enough. It's not mlocked memory, it's mlocked memory and I can exfiltrate it. Mlock is fine, exfiltration not so much. It's mlock, but a global pool and if you didn't munlock then the next mlock from a completely different user will alias with your stuff. Or is there something that prevents that? Oded at least explain that gaudi works like a gpu from 20 years ago, single user, no security at all within the device. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 8 months

2
2
0 0

Re: [Linaro-mm-sig] [PATCH v6 0/2] Add p2p via dmabuf to habanalabs

by Daniel Vetter

On Thu, Sep 16, 2021 at 03:44:25PM +0300, Oded Gabbay wrote: > On Thu, Sep 16, 2021 at 3:31 PM Daniel Vetter <daniel(a)ffwll.ch> wrote: > > > > On Wed, Sep 15, 2021 at 10:45:36AM +0300, Oded Gabbay wrote: > > > On Tue, Sep 14, 2021 at 7:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > > > > > On Tue, Sep 14, 2021 at 04:18:31PM +0200, Daniel Vetter wrote: > > > > > On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > > > > > > Hi, > > > > > > Re-sending this patch-set following the release of our user-space TPC > > > > > > compiler and runtime library. > > > > > > > > > > > > I would appreciate a review on this. > > > > > > > > > > I think the big open we have is the entire revoke discussions. Having the > > > > > option to let dma-buf hang around which map to random local memory ranges, > > > > > without clear ownership link and a way to kill it sounds bad to me. > > > > > > > > > > I think there's a few options: > > > > > - We require revoke support. But I've heard rdma really doesn't like that, > > > > > I guess because taking out an MR while holding the dma_resv_lock would > > > > > be an inversion, so can't be done. Jason, can you recap what exactly the > > > > > hold-up was again that makes this a no-go? > > > > > > > > RDMA HW can't do revoke. > > > > Like why? I'm assuming when the final open handle or whatever for that MR > > is closed, you do clean up everything? Or does that MR still stick around > > forever too? > > > > > > So we have to exclude almost all the HW and several interesting use > > > > cases to enable a revoke operation. > > > > > > > > > - For non-revokable things like these dma-buf we'd keep a drm_master > > > > > reference around. This would prevent the next open to acquire > > > > > ownership rights, which at least prevents all the nasty potential > > > > > problems. > > > > > > > > This is what I generally would expect, the DMABUF FD and its DMA > > > > memory just floats about until the unrevokable user releases it, which > > > > happens when the FD that is driving the import eventually gets closed. > > > This is exactly what we are doing in the driver. We make sure > > > everything is valid until the unrevokable user releases it and that > > > happens only when the dmabuf fd gets closed. > > > And the user can't close it's fd of the device until he performs the > > > above, so there is no leakage between users. > > > > Maybe I got the device security model all wrong, but I thought Guadi is > > single user, and the only thing it protects is the system against the > > Gaudi device trhough iommu/device gart. So roughly the following can > > happen: > > > > 1. User A opens gaudi device, sets up dma-buf export > > > > 2. User A registers that with RDMA, or anything else that doesn't support > > revoke. > > > > 3. User A closes gaudi device > This can not happen without User A closing the FD of the dma-buf it exported. > We prevent User A from closing the device because when it exported the > dma-buf, the driver's code took a refcnt of the user's private > structure. You can see that in export_dmabuf_common() in the 2nd > patch. There is a call there to hl_ctx_get. > So even if User A calls close(device_fd), the driver won't let any > other user open the device until User A closes the fd of the dma-buf > object. > > Moreover, once User A will close the dma-buf fd and the device is > released, the driver will scrub the device memory (this is optional > for systems who care about security). > > And AFAIK, User A can't close the dma-buf fd once it registered it > with RDMA, without doing unregister. > This can be seen in ib_umem_dmabuf_get() which calls dma_buf_get() > which does fget(fd) Yeah that's essentially what I was looking for. This is defacto hand-rolling the drm_master owner tracking stuff. As long as we have something like this in place it should be fine I think. -Daniel > > 4. User B opens gaudi device, assumes that it has full control over the > > device and uploads some secrets, which happen to end up in the dma-buf > > region user A set up > > > > 5. User B extracts secrets. > > > > > > I still don't think any of the complexity is needed, pinnable memory > > > > is a thing in Linux, just account for it in mlocked and that is > > > > enough. > > > > It's not mlocked memory, it's mlocked memory and I can exfiltrate it. > > Mlock is fine, exfiltration not so much. It's mlock, but a global pool and > > if you didn't munlock then the next mlock from a completely different user > > will alias with your stuff. > > > > Or is there something that prevents that? Oded at least explain that gaudi > > works like a gpu from 20 years ago, single user, no security at all within > > the device. > > -Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 8 months

1
0
0 0

[PATCH 01/14] dma-buf: add dma_resv_for_each_fence_unlocked

by Christian König

Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-resv.c | 63 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 36 ++++++++++++++++++++++ 2 files changed, 99 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..213a9b7251ca 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,69 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iterator is started over again. + */ +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool all_fences, bool first) +{ + struct dma_fence *fence = NULL; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(fence); + + cursor->is_first = first; + if (first) { + cursor->seq = read_seqcount_begin(&obj->seq); + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + cursor->is_exclusive = true; + + fence = dma_resv_excl_fence(obj); + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + fence = NULL; + } else { + fence = NULL; + } + + if (fence) { + fence = dma_fence_get_rcu(fence); + } else if (all_fences && cursor->fences) { + struct dma_resv_list *fences = cursor->fences; + + cursor->is_exclusive = false; + while (++cursor->index < fences->shared_count) { + fence = rcu_dereference(fences->shared[ + cursor->index]); + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + break; + } + if (cursor->index < fences->shared_count) + fence = dma_fence_get_rcu(fence); + else + fence = NULL; + } + + /* For the eventually next round */ + first = true; + } while (read_seqcount_retry(&obj->seq, cursor->seq)); + + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..f5b91c292ee0 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,39 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_cursor - current position into the dma_resv fences + * @seq: sequence number to check + * @index: index into the shared fences + * @shared: the shared fences + * @is_first: true if this is the first returned fence + * @is_exclusive: if the current fence is the exclusive one + */ +struct dma_resv_cursor { + unsigned int seq; + unsigned int index; + struct dma_resv_list *fences; + bool is_first; + bool is_exclusive; +}; + +/** + * dma_resv_for_each_fence_unlocked - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_cursor pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * dma_resv::lock. The RCU read side lock must be hold when using this, but can + * be dropped and re-taken as necessary inside the loop. @all_fences controls + * if the shared fences are returned as well. + */ +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) @@ -366,6 +399,9 @@ void dma_resv_fini(struct dma_resv *obj); int dma_resv_reserve_shared(struct dma_resv *obj, unsigned int num_fences); void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence); void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_cursor *cursor, + bool first, bool all_fences); int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, unsigned *pshared_count, struct dma_fence ***pshared); int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); -- 2.25.1

3 years, 8 months

3
22
0 0

Deploying new iterator interface for dma-buf

by Christian König

Next round for that one here, maybe the CI systems are now more gracefully with me :) I'm pretty sure that a couple of those dma_resv_for_each_fence_unlocked should actually be replaced with lock+dma_resv_for_each_fence, but that needs more auditing. Please review and comment. Thanks, Christian.

3 years, 8 months

2
27
0 0

Deploying new iterator interface for dma-buf

by Christian König

Hi everybody, we recently found that a good bunch of the RCU accesses to the dma_resv object are actually not correctly protected. Those where fixed by either dropping the RCU approach and taking appropriate locks or using a central function to return the current fences as array and then work with that snapshot. This set now tries to prevent adding any new broken code by rolling out two new interfaces to access the fences in a dma_resv object: dma_resv_for_each_fence() - Iterator which should be used while holding the reservation lock. dma_resv_for_each_fence_unlocked() - Iterator based on RCU which can be used without holding the reservation lock and automatic restart on concurrent modification. While doing this we also move the decision which fences to use for write and read accesses into the dma_resv object which results in a quite nice code de-duplication and simplification. The only two remaining users of the RCU shared fence interface are removing shared fences in amdkfd and debugfs code in qxl which will both be addresses in the next patch set. Please review and/or comment, Christian.

3 years, 8 months

2
39
0 0

Re: [Linaro-mm-sig] [PATCH v6 0/2] Add p2p via dmabuf to habanalabs

by Daniel Vetter

On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > Hi, > Re-sending this patch-set following the release of our user-space TPC > compiler and runtime library. > > I would appreciate a review on this. I think the big open we have is the entire revoke discussions. Having the option to let dma-buf hang around which map to random local memory ranges, without clear ownership link and a way to kill it sounds bad to me. I think there's a few options: - We require revoke support. But I've heard rdma really doesn't like that, I guess because taking out an MR while holding the dma_resv_lock would be an inversion, so can't be done. Jason, can you recap what exactly the hold-up was again that makes this a no-go? - The other option I discussed is a bit more the exlusive device ownership model we've had for gpus in drm of the really old kind. Roughly this would work like this, in terms of drm_device: - Only the current owner (drm_master in current drm code, but should probably rename that to drm_owner) is allowed to use the accel driver. So all ioctl would fail if you're not drm_master. - On dropmaster/file close we'd revoke as much as possible, e.g. in-flight commands, mmaps, anything really that can be revoked. - For non-revokable things like these dma-buf we'd keep a drm_master reference around. This would prevent the next open to acquire ownership rights, which at least prevents all the nasty potential problems. - admin (or well container orchestrator) then has responsibility to shoot down all process until the problem goes away (i.e. until you hit the one with the rdma MR which keeps the dma-buf alive) - Not sure there's another reasonable way to do this without inviting some problems once we get outside of the "single kernel instance per tenant" use-case. Wrt implementation there's the trouble of this reinventing a bunch of drm stuff and concepts, but that's maybe for after we've figured out semantics. Also would be great if you have a pull request for the userspace runtime that shows a bit how this all gets used and tied together. Or maybe some pointers, since I guess retconning a PR in github is maybe a bit much. Cheers, Daniel > > Thanks, > Oded > > Oded Gabbay (1): > habanalabs: define uAPI to export FD for DMA-BUF > > Tomer Tayar (1): > habanalabs: add support for dma-buf exporter > > drivers/misc/habanalabs/Kconfig | 1 + > drivers/misc/habanalabs/common/habanalabs.h | 22 + > drivers/misc/habanalabs/common/memory.c | 522 +++++++++++++++++++- > drivers/misc/habanalabs/gaudi/gaudi.c | 1 + > drivers/misc/habanalabs/goya/goya.c | 1 + > include/uapi/misc/habanalabs.h | 28 +- > 6 files changed, 570 insertions(+), 5 deletions(-) > > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 8 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 0/9] dma-fence: Deadline awareness

by Rob Clark

On Thu, Sep 9, 2021 at 9:42 AM Simon Ser <contact(a)emersion.fr> wrote: > > On Thursday, September 9th, 2021 at 18:31, Rob Clark <robdclark(a)gmail.com> wrote: > > > Yes, I think it would.. and "dma-buf/sync_file: Add SET_DEADLINE > > ioctl" adds such an ioctl.. just for the benefit of igt tests at this > > point, but the thought was it would be also used by compositors that > > are doing such frame scheduling. Ofc danvet is a bit grumpy that > > there isn't a more real (than igt) userspace for the ioctl yet ;-) > > Ah, very nice, I somehow missed it. > > I guess one issue is that explicit sync isn't quite plumbed through > compositors yet, so without Jason's DMA-BUF to sync_file IOCTL it'd be > a bit difficult to use. > > Can anybody set the deadline? I wonder if clients should be allowed to. In its current form, anyone who has the fd can.. I'm not sure how (or even if) we could limit it beyond that. I suppose hypothetically you could use this for completely non-compositor related things, like compute jobs where you want the result by some deadline. (OTOH, it could be the driver using this internally when the app is stalling on a result) > What happens if the deadline is exceeded? I'd assume nothing in > particular, the deadline being just a hint? Nothing in particular, it is just a hint. The main intention is to provide a feedback hint to the drivers in scenarios like vblank deadlines, where being 1ms late is just as bad as being $frame_duration-1ms late. From my experiments and profiling it is useful in a couple scenarios: 1) input latency, ie. go from idle to fullscreen animation, where GPU has been idle for a while and not busy enough *yet* for devfreq to kick in and start ramping up the freq before we miss the first vblank 2) double buffering, for ex. if you are 1ms late you end up stalling 15ms before the gpu can start rendering the next frame.. in the absence of feedback, devfreq would ramp down in this scenario instead of up BR, -R

3 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 0/9] dma-fence: Deadline awareness

by Rob Clark

On Thu, Sep 9, 2021 at 9:16 AM Simon Ser <contact(a)emersion.fr> wrote: > > Out of curiosity, would it be reasonable to allow user-space (more > precisely, the compositor) to set the deadline via an IOCTL without > actually performing an atomic commit with the FB? > > Some compositors might want to wait themselves for FB fence completions > to ensure a client doesn't block the whole desktop (by submitting a > very costly rendering job). In this case it would make sense for the > compositor to indicate that it intends to display the buffer on next > vblank if it's ready by that point, without queueing a page-flip yet. Yes, I think it would.. and "dma-buf/sync_file: Add SET_DEADLINE ioctl" adds such an ioctl.. just for the benefit of igt tests at this point, but the thought was it would be also used by compositors that are doing such frame scheduling. Ofc danvet is a bit grumpy that there isn't a more real (than igt) userspace for the ioctl yet ;-) BR, -R

3 years, 9 months

1
0
0 0

[PATCH AUTOSEL 5.14 010/252] dma-buf: fix dma_resv_test_signaled test_all handling v2

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 9d38814d1e346ea37a51cbf31f4424c9d059459e ] As the name implies if testing all fences is requested we should indeed test all fences and not skip the exclusive one because we see shared ones. v2: fix logic once more Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210702111642.17259-3-christ… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-resv.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index f26c71747d43..e744fd87c63c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -615,25 +615,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { - unsigned int seq, shared_count; + struct dma_fence *fence; + unsigned int seq; int ret; rcu_read_lock(); retry: ret = true; - shared_count = 0; seq = read_seqcount_begin(&obj->seq); if (test_all) { struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i; - - if (fobj) - shared_count = fobj->shared_count; + unsigned int i, shared_count; + shared_count = fobj ? fobj->shared_count : 0; for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence; - fence = rcu_dereference(fobj->shared[i]); ret = dma_resv_test_signaled_single(fence); if (ret < 0) @@ -641,24 +637,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) else if (!ret) break; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; } - if (!shared_count) { - struct dma_fence *fence_excl = dma_resv_excl_fence(obj); - - if (fence_excl) { - ret = dma_resv_test_signaled_single(fence_excl); - if (ret < 0) - goto retry; + fence = dma_resv_excl_fence(obj); + if (ret && fence) { + ret = dma_resv_test_signaled_single(fence); + if (ret < 0) + goto retry; - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - } } + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; + rcu_read_unlock(); return ret; } -- 2.30.2

3 years, 9 months

1
0
0 0

[PATCH AUTOSEL 5.14 010/252] dma-buf: fix dma_resv_test_signaled test_all handling v2

by Sasha Levin

From: Christian König <christian.koenig(a)amd.com> [ Upstream commit 9d38814d1e346ea37a51cbf31f4424c9d059459e ] As the name implies if testing all fences is requested we should indeed test all fences and not skip the exclusive one because we see shared ones. v2: fix logic once more Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210702111642.17259-3-christ… Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/dma-buf/dma-resv.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index f26c71747d43..e744fd87c63c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -615,25 +615,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { - unsigned int seq, shared_count; + struct dma_fence *fence; + unsigned int seq; int ret; rcu_read_lock(); retry: ret = true; - shared_count = 0; seq = read_seqcount_begin(&obj->seq); if (test_all) { struct dma_resv_list *fobj = dma_resv_shared_list(obj); - unsigned int i; - - if (fobj) - shared_count = fobj->shared_count; + unsigned int i, shared_count; + shared_count = fobj ? fobj->shared_count : 0; for (i = 0; i < shared_count; ++i) { - struct dma_fence *fence; - fence = rcu_dereference(fobj->shared[i]); ret = dma_resv_test_signaled_single(fence); if (ret < 0) @@ -641,24 +637,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) else if (!ret) break; } - - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; } - if (!shared_count) { - struct dma_fence *fence_excl = dma_resv_excl_fence(obj); - - if (fence_excl) { - ret = dma_resv_test_signaled_single(fence_excl); - if (ret < 0) - goto retry; + fence = dma_resv_excl_fence(obj); + if (ret && fence) { + ret = dma_resv_test_signaled_single(fence); + if (ret < 0) + goto retry; - if (read_seqcount_retry(&obj->seq, seq)) - goto retry; - } } + if (read_seqcount_retry(&obj->seq, seq)) + goto retry; + rcu_read_unlock(); return ret; } -- 2.30.2

3 years, 9 months

1
0
0 0

Harden the dma-fence documentation a bit more

by Christian König

Hi guys, while it is in most cases technically possible to not have a reference to the dma_fence when adding a callback it is usually a good idea to make sure to always have a reference anyway. Otherwise we can indeed see cases where this doesn't really work as intended like for example in the now fixed EPOLL code. Regards, Christian.

3 years, 9 months

2
5
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: Add support for mapping buffers with DMA attributes

by Daniel Vetter

On Mon, Aug 30, 2021 at 10:39:11AM +0800, guangming.cao(a)mediatek.com wrote: > From: Guangming Cao <Guangming.Cao(a)mediatek.com> > > When mapping the memory represented by a dma-buf into a device's > address space, it might be desireable to map the memory with > certain DMA attributes. Thus, introduce the dma_mapping_attrs > field in the dma_buf_attachment structure so that when > the memory is mapped with dma_buf_map_attachment, it is mapped > with the desired DMA attributes. > > Signed-off-by: Isaac J. Manjarres <isaacm(a)codeaurora.org> > Signed-off-by: Sandeep Patil <sspatil(a)google.com> > Signed-off-by: Guangming Cao <Guangming.Cao(a)mediatek.com> Can you pls include the code that's going to use this here too? At a glance all the attributes you might want to set are supposed to be under the control of the exporter, not the importer. -Daniel > --- > drivers/dma-buf/heaps/cma_heap.c | 6 ++++-- > drivers/dma-buf/heaps/system_heap.c | 6 ++++-- > include/linux/dma-buf.h | 3 +++ > 3 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c > index 0c05b79870f9..2c9feb3bfc3e 100644 > --- a/drivers/dma-buf/heaps/cma_heap.c > +++ b/drivers/dma-buf/heaps/cma_heap.c > @@ -99,9 +99,10 @@ static struct sg_table *cma_heap_map_dma_buf(struct dma_buf_attachment *attachme > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = &a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(-ENOMEM); > a->mapped = true; > @@ -113,9 +114,10 @@ static void cma_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int cma_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c > index 23a7e74ef966..fc7b1e02988e 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -130,9 +130,10 @@ static struct sg_table *system_heap_map_dma_buf(struct dma_buf_attachment *attac > { > struct dma_heap_attachment *a = attachment->priv; > struct sg_table *table = a->table; > + int attrs = attachment->dma_map_attrs; > int ret; > > - ret = dma_map_sgtable(attachment->dev, table, direction, 0); > + ret = dma_map_sgtable(attachment->dev, table, direction, attrs); > if (ret) > return ERR_PTR(ret); > > @@ -145,9 +146,10 @@ static void system_heap_unmap_dma_buf(struct dma_buf_attachment *attachment, > enum dma_data_direction direction) > { > struct dma_heap_attachment *a = attachment->priv; > + int attrs = attachment->dma_map_attrs; > > a->mapped = false; > - dma_unmap_sgtable(attachment->dev, table, direction, 0); > + dma_unmap_sgtable(attachment->dev, table, direction, attrs); > } > > static int system_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index efdc56b9d95f..4d650731766e 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -379,6 +379,8 @@ struct dma_buf_attach_ops { > * @importer_ops: importer operations for this attachment, if provided > * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. > * @importer_priv: importer specific attachment data. > + * @dma_map_attrs: DMA attributes to be used when the exporter maps the buffer > + * through dma_buf_map_attachment. > * > * This structure holds the attachment information between the dma_buf buffer > * and its user device(s). The list contains one attachment struct per device > @@ -399,6 +401,7 @@ struct dma_buf_attachment { > const struct dma_buf_attach_ops *importer_ops; > void *importer_priv; > void *priv; > + unsigned long dma_map_attrs; > }; > > /** > -- > 2.17.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 9 months

1
0
0 0

[PATCH 1/2] dma-buf: nuke DMA_FENCE_TRACE macros v2

by Christian König

Only the DRM GPU scheduler, radeon and amdgpu where using them and they depend on a non existing config option to actually emit some code. v2: keep the signal path as is for now Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 10 +--------- drivers/gpu/drm/radeon/radeon_fence.c | 24 ++++------------------- drivers/gpu/drm/scheduler/sched_fence.c | 18 ++--------------- include/linux/dma-fence.h | 22 --------------------- 4 files changed, 7 insertions(+), 67 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index 0b1c48590c43..c65994e382bd 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -246,7 +246,6 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) struct amdgpu_fence_driver *drv = &ring->fence_drv; struct amdgpu_device *adev = ring->adev; uint32_t seq, last_seq; - int r; do { last_seq = atomic_read(&ring->fence_drv.last_seq); @@ -278,12 +277,7 @@ bool amdgpu_fence_process(struct amdgpu_ring *ring) if (!fence) continue; - r = dma_fence_signal(fence); - if (!r) - DMA_FENCE_TRACE(fence, "signaled from irq context\n"); - else - BUG(); - + dma_fence_signal(fence); dma_fence_put(fence); pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); @@ -639,8 +633,6 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) if (!timer_pending(&ring->fence_drv.fallback_timer)) amdgpu_fence_schedule_fallback(ring); - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", ring->idx); - return true; } diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c index 18f2c2e0dfb3..3f351d222cbb 100644 --- a/drivers/gpu/drm/radeon/radeon_fence.c +++ b/drivers/gpu/drm/radeon/radeon_fence.c @@ -176,18 +176,11 @@ static int radeon_fence_check_signaled(wait_queue_entry_t *wait, unsigned mode, */ seq = atomic64_read(&fence->rdev->fence_drv[fence->ring].last_seq); if (seq >= fence->seq) { - int ret = dma_fence_signal_locked(&fence->base); - - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->base, "was already signaled\n"); - + dma_fence_signal_locked(&fence->base); radeon_irq_kms_sw_irq_put(fence->rdev, fence->ring); __remove_wait_queue(&fence->rdev->fence_queue, &fence->fence_wake); dma_fence_put(&fence->base); - } else - DMA_FENCE_TRACE(&fence->base, "pending\n"); + } return 0; } @@ -422,8 +415,6 @@ static bool radeon_fence_enable_signaling(struct dma_fence *f) fence->fence_wake.func = radeon_fence_check_signaled; __add_wait_queue(&rdev->fence_queue, &fence->fence_wake); dma_fence_get(f); - - DMA_FENCE_TRACE(&fence->base, "armed on ring %i!\n", fence->ring); return true; } @@ -441,11 +432,7 @@ bool radeon_fence_signaled(struct radeon_fence *fence) return true; if (radeon_fence_seq_signaled(fence->rdev, fence->seq, fence->ring)) { - int ret; - - ret = dma_fence_signal(&fence->base); - if (!ret) - DMA_FENCE_TRACE(&fence->base, "signaled from radeon_fence_signaled\n"); + dma_fence_signal(&fence->base); return true; } return false; @@ -550,7 +537,6 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo { uint64_t seq[RADEON_NUM_RINGS] = {}; long r; - int r_sig; /* * This function should not be called on !radeon fences. @@ -567,9 +553,7 @@ long radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeo return r; } - r_sig = dma_fence_signal(&fence->base); - if (!r_sig) - DMA_FENCE_TRACE(&fence->base, "signaled from fence_wait\n"); + dma_fence_signal(&fence->base); return r; } diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..3736746c47bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -50,26 +50,12 @@ static void __exit drm_sched_fence_slab_fini(void) void drm_sched_fence_scheduled(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->scheduled); - - if (!ret) - DMA_FENCE_TRACE(&fence->scheduled, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->scheduled, - "was already signaled\n"); + dma_fence_signal(&fence->scheduled); } void drm_sched_fence_finished(struct drm_sched_fence *fence) { - int ret = dma_fence_signal(&fence->finished); - - if (!ret) - DMA_FENCE_TRACE(&fence->finished, - "signaled from irq context\n"); - else - DMA_FENCE_TRACE(&fence->finished, - "was already signaled\n"); + dma_fence_signal(&fence->finished); } static const char *drm_sched_fence_get_driver_name(struct dma_fence *fence) diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 6ffb4b2c6371..4cc119ab272f 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -590,26 +590,4 @@ struct dma_fence *dma_fence_get_stub(void); struct dma_fence *dma_fence_allocate_private_stub(void); u64 dma_fence_context_alloc(unsigned num); -#define DMA_FENCE_TRACE(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - if (IS_ENABLED(CONFIG_DMA_FENCE_TRACE)) \ - pr_info("f %llu#%llu: " fmt, \ - __ff->context, __ff->seqno, ##args); \ - } while (0) - -#define DMA_FENCE_WARN(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_warn("f %llu#%llu: " fmt, __ff->context, __ff->seqno,\ - ##args); \ - } while (0) - -#define DMA_FENCE_ERR(f, fmt, args...) \ - do { \ - struct dma_fence *__ff = (f); \ - pr_err("f %llu#%llu: " fmt, __ff->context, __ff->seqno, \ - ##args); \ - } while (0) - #endif /* __LINUX_DMA_FENCE_H */ -- 2.25.1

3 years, 9 months

2
9
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig