| Message ID | 20220407085946.744568-13-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] dma-buf: add enum dma_resv_usage v4 | expand |
On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: > That should now be handled by the common dma_resv framework. > > Signed-off-by: Christian König <christian.koenig@amd.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: intel-gfx@lists.freedesktop.org So, where are the i915 maintainer acks for merging this (and the other patches in the series touching i915) via drm-misc-next? Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. We don't merge i915 stuff without passing CI results. Apparently this one failed enough machines that the CI had to be stopped entirely. BR, Jani. > --- > drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- > drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ > .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- > .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- > drivers/gpu/drm/i915/i915_vma.c | 9 +++- > 6 files changed, 21 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > index 372bc220faeb..ffde7bc0a95d 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { > /** > * i915_gem_object_get_moving_fence - Get the object's moving fence if any > * @obj: The object whose moving fence to get. > + * @fence: The resulting fence > * > * A non-signaled moving fence means that there is an async operation > * pending on the object that needs to be waited on before setting up > * any GPU- or CPU PTEs to the object's pages. > * > - * Return: A refcounted pointer to the object's moving fence if any, > - * NULL otherwise. > + * Return: Negative error code or 0 for success. > */ > -struct dma_fence * > -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) > +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, > + struct dma_fence **fence) > { > - return dma_fence_get(i915_gem_to_ttm(obj)->moving); > -} > - > -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > - struct dma_fence *fence) > -{ > - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; > - > - if (*moving == fence) > - return; > - > - dma_fence_put(*moving); > - *moving = dma_fence_get(fence); > + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, > + fence); > } > > /** > @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, > bool intr) > { > - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; > - int ret; > - > assert_object_held(obj); > - if (!fence) > - return 0; > - > - ret = dma_fence_wait(fence, intr); > - if (ret) > - return ret; > - > - if (fence->error) > - return fence->error; > - > - i915_gem_to_ttm(obj)->moving = NULL; > - dma_fence_put(fence); > - return 0; > + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, > + intr, MAX_SCHEDULE_TIMEOUT); > } > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > index 02c37fe4a535..e11d82a9f7c3 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) > i915_gem_object_unpin_pages(obj); > } > > -struct dma_fence * > -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); > - > -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > - struct dma_fence *fence); > - > +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, > + struct dma_fence **fence); > int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, > bool intr); > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > index 438b8a95b3d1..a10716f4e717 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, > return fence; > } > > -static int > -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, > - struct i915_deps *deps) > -{ > - int ret; > - > - ret = i915_deps_add_dependency(deps, bo->moving, ctx); > - if (!ret) > - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); > - > - return ret; > -} > - > /** > * i915_ttm_move - The TTM move callback used by i915. > * @bo: The buffer object. > @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, > struct i915_deps deps; > > i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); > - ret = prev_deps(bo, ctx, &deps); > + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); > if (ret) { > i915_refct_sgt_put(dst_rsgt); > return ret; > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > index 4997ed18b6e4..0ad443a90c8b 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, > err = dma_resv_reserve_fences(obj->base.resv, 1); > if (!err) > dma_resv_add_fence(obj->base.resv, &rq->fence, > - DMA_RESV_USAGE_WRITE); > - i915_gem_object_set_moving_fence(obj, &rq->fence); > + DMA_RESV_USAGE_KERNEL); > i915_request_put(rq); > } > if (err) > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > index 3a6e3f6d239f..dfc34cc2ef8c 100644 > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, > i915_gem_object_unpin_pages(obj); > if (rq) { > dma_resv_add_fence(obj->base.resv, &rq->fence, > - DMA_RESV_USAGE_WRITE); > - i915_gem_object_set_moving_fence(obj, &rq->fence); > + DMA_RESV_USAGE_KERNEL); > i915_request_put(rq); > } > i915_gem_object_unlock(obj); > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index 524477d8939e..d077f7b9eaad 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, > if (err) > return err; > > + if (vma->obj) { > + err = i915_gem_object_get_moving_fence(vma->obj, &moving); > + if (err) > + return err; > + } else { > + moving = NULL; > + } > + > if (flags & PIN_GLOBAL) > wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); > > - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; > if (flags & vma->vm->bind_async_flags || moving) { > /* lock VM */ > err = i915_vm_lock_objects(vma->vm, ww);
Am 08.04.22 um 11:05 schrieb Jani Nikula: > On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: >> That should now be handled by the common dma_resv framework. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: intel-gfx@lists.freedesktop.org > So, where are the i915 maintainer acks for merging this (and the other > patches in the series touching i915) via drm-misc-next? > > Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. I had the impression that it would be sufficient. > We don't merge i915 stuff without passing CI results. Apparently this > one failed enough machines that the CI had to be stopped entirely. That was unfortunately partially expected and pointed out by Matthew and Daniel before the push. i915 for some reason extended the usage of the bo->moving fence despite the fact we had patches on the mailing list to entirely remove this feature. I couldn't get any sane CI results for weeks because of this and at some point we just had to go ahead and fix the clash in drm-tip. Sorry for any inconvenience cause by that. I hoped that we fixed all cases, but looks like we still missed some. Regards, Christian. > > > BR, > Jani. > > >> --- >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- >> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- >> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ >> .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- >> .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- >> drivers/gpu/drm/i915/i915_vma.c | 9 +++- >> 6 files changed, 21 insertions(+), 58 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> index 372bc220faeb..ffde7bc0a95d 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >> @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { >> /** >> * i915_gem_object_get_moving_fence - Get the object's moving fence if any >> * @obj: The object whose moving fence to get. >> + * @fence: The resulting fence >> * >> * A non-signaled moving fence means that there is an async operation >> * pending on the object that needs to be waited on before setting up >> * any GPU- or CPU PTEs to the object's pages. >> * >> - * Return: A refcounted pointer to the object's moving fence if any, >> - * NULL otherwise. >> + * Return: Negative error code or 0 for success. >> */ >> -struct dma_fence * >> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) >> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >> + struct dma_fence **fence) >> { >> - return dma_fence_get(i915_gem_to_ttm(obj)->moving); >> -} >> - >> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >> - struct dma_fence *fence) >> -{ >> - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; >> - >> - if (*moving == fence) >> - return; >> - >> - dma_fence_put(*moving); >> - *moving = dma_fence_get(fence); >> + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, >> + fence); >> } >> >> /** >> @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >> bool intr) >> { >> - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; >> - int ret; >> - >> assert_object_held(obj); >> - if (!fence) >> - return 0; >> - >> - ret = dma_fence_wait(fence, intr); >> - if (ret) >> - return ret; >> - >> - if (fence->error) >> - return fence->error; >> - >> - i915_gem_to_ttm(obj)->moving = NULL; >> - dma_fence_put(fence); >> - return 0; >> + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, >> + intr, MAX_SCHEDULE_TIMEOUT); >> } >> >> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h >> index 02c37fe4a535..e11d82a9f7c3 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >> @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) >> i915_gem_object_unpin_pages(obj); >> } >> >> -struct dma_fence * >> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); >> - >> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >> - struct dma_fence *fence); >> - >> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >> + struct dma_fence **fence); >> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >> bool intr); >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> index 438b8a95b3d1..a10716f4e717 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >> @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, >> return fence; >> } >> >> -static int >> -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, >> - struct i915_deps *deps) >> -{ >> - int ret; >> - >> - ret = i915_deps_add_dependency(deps, bo->moving, ctx); >> - if (!ret) >> - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); >> - >> - return ret; >> -} >> - >> /** >> * i915_ttm_move - The TTM move callback used by i915. >> * @bo: The buffer object. >> @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, >> struct i915_deps deps; >> >> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); >> - ret = prev_deps(bo, ctx, &deps); >> + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); >> if (ret) { >> i915_refct_sgt_put(dst_rsgt); >> return ret; >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >> index 4997ed18b6e4..0ad443a90c8b 100644 >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >> @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, >> err = dma_resv_reserve_fences(obj->base.resv, 1); >> if (!err) >> dma_resv_add_fence(obj->base.resv, &rq->fence, >> - DMA_RESV_USAGE_WRITE); >> - i915_gem_object_set_moving_fence(obj, &rq->fence); >> + DMA_RESV_USAGE_KERNEL); >> i915_request_put(rq); >> } >> if (err) >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >> index 3a6e3f6d239f..dfc34cc2ef8c 100644 >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >> @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, >> i915_gem_object_unpin_pages(obj); >> if (rq) { >> dma_resv_add_fence(obj->base.resv, &rq->fence, >> - DMA_RESV_USAGE_WRITE); >> - i915_gem_object_set_moving_fence(obj, &rq->fence); >> + DMA_RESV_USAGE_KERNEL); >> i915_request_put(rq); >> } >> i915_gem_object_unlock(obj); >> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >> index 524477d8939e..d077f7b9eaad 100644 >> --- a/drivers/gpu/drm/i915/i915_vma.c >> +++ b/drivers/gpu/drm/i915/i915_vma.c >> @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, >> if (err) >> return err; >> >> + if (vma->obj) { >> + err = i915_gem_object_get_moving_fence(vma->obj, &moving); >> + if (err) >> + return err; >> + } else { >> + moving = NULL; >> + } >> + >> if (flags & PIN_GLOBAL) >> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); >> >> - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; >> if (flags & vma->vm->bind_async_flags || moving) { >> /* lock VM */ >> err = i915_vm_lock_objects(vma->vm, ww);
On Fri, 8 Apr 2022 at 11:27, Christian König <christian.koenig@amd.com> wrote: > > Am 08.04.22 um 11:05 schrieb Jani Nikula: > > On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: > >> That should now be handled by the common dma_resv framework. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: intel-gfx@lists.freedesktop.org > > So, where are the i915 maintainer acks for merging this (and the other > > patches in the series touching i915) via drm-misc-next? > > > > Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. > > I had the impression that it would be sufficient. > > > We don't merge i915 stuff without passing CI results. Apparently this > > one failed enough machines that the CI had to be stopped entirely. > > That was unfortunately partially expected and pointed out by Matthew and > Daniel before the push. Uh I didn't realize that CI never tested this. Usually the way then is to rebase onto drm-tip and figure out the merge story. Doing subsystem wide changes while not on linux-next or drm-tip or another integration tree is just fraught with surprises due to conflicts. Also "doesn't even compile" is really below the bar, and not the first time this happened at all. And i915 isn't really an obscure driver which is hard to compile test :-) -Daniel > i915 for some reason extended the usage of the bo->moving fence despite > the fact we had patches on the mailing list to entirely remove this feature. > > I couldn't get any sane CI results for weeks because of this and at some > point we just had to go ahead and fix the clash in drm-tip. > > Sorry for any inconvenience cause by that. I hoped that we fixed all > cases, but looks like we still missed some. > > Regards, > Christian. > > > > > > > BR, > > Jani. > > > > > >> --- > >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- > >> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- > >> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ > >> .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- > >> .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- > >> drivers/gpu/drm/i915/i915_vma.c | 9 +++- > >> 6 files changed, 21 insertions(+), 58 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c > >> index 372bc220faeb..ffde7bc0a95d 100644 > >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c > >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c > >> @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { > >> /** > >> * i915_gem_object_get_moving_fence - Get the object's moving fence if any > >> * @obj: The object whose moving fence to get. > >> + * @fence: The resulting fence > >> * > >> * A non-signaled moving fence means that there is an async operation > >> * pending on the object that needs to be waited on before setting up > >> * any GPU- or CPU PTEs to the object's pages. > >> * > >> - * Return: A refcounted pointer to the object's moving fence if any, > >> - * NULL otherwise. > >> + * Return: Negative error code or 0 for success. > >> */ > >> -struct dma_fence * > >> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) > >> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, > >> + struct dma_fence **fence) > >> { > >> - return dma_fence_get(i915_gem_to_ttm(obj)->moving); > >> -} > >> - > >> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > >> - struct dma_fence *fence) > >> -{ > >> - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; > >> - > >> - if (*moving == fence) > >> - return; > >> - > >> - dma_fence_put(*moving); > >> - *moving = dma_fence_get(fence); > >> + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, > >> + fence); > >> } > >> > >> /** > >> @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > >> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, > >> bool intr) > >> { > >> - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; > >> - int ret; > >> - > >> assert_object_held(obj); > >> - if (!fence) > >> - return 0; > >> - > >> - ret = dma_fence_wait(fence, intr); > >> - if (ret) > >> - return ret; > >> - > >> - if (fence->error) > >> - return fence->error; > >> - > >> - i915_gem_to_ttm(obj)->moving = NULL; > >> - dma_fence_put(fence); > >> - return 0; > >> + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, > >> + intr, MAX_SCHEDULE_TIMEOUT); > >> } > >> > >> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h > >> index 02c37fe4a535..e11d82a9f7c3 100644 > >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h > >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h > >> @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) > >> i915_gem_object_unpin_pages(obj); > >> } > >> > >> -struct dma_fence * > >> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); > >> - > >> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, > >> - struct dma_fence *fence); > >> - > >> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, > >> + struct dma_fence **fence); > >> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, > >> bool intr); > >> > >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > >> index 438b8a95b3d1..a10716f4e717 100644 > >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c > >> @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, > >> return fence; > >> } > >> > >> -static int > >> -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, > >> - struct i915_deps *deps) > >> -{ > >> - int ret; > >> - > >> - ret = i915_deps_add_dependency(deps, bo->moving, ctx); > >> - if (!ret) > >> - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); > >> - > >> - return ret; > >> -} > >> - > >> /** > >> * i915_ttm_move - The TTM move callback used by i915. > >> * @bo: The buffer object. > >> @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, > >> struct i915_deps deps; > >> > >> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); > >> - ret = prev_deps(bo, ctx, &deps); > >> + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); > >> if (ret) { > >> i915_refct_sgt_put(dst_rsgt); > >> return ret; > >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > >> index 4997ed18b6e4..0ad443a90c8b 100644 > >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c > >> @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, > >> err = dma_resv_reserve_fences(obj->base.resv, 1); > >> if (!err) > >> dma_resv_add_fence(obj->base.resv, &rq->fence, > >> - DMA_RESV_USAGE_WRITE); > >> - i915_gem_object_set_moving_fence(obj, &rq->fence); > >> + DMA_RESV_USAGE_KERNEL); > >> i915_request_put(rq); > >> } > >> if (err) > >> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > >> index 3a6e3f6d239f..dfc34cc2ef8c 100644 > >> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > >> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > >> @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, > >> i915_gem_object_unpin_pages(obj); > >> if (rq) { > >> dma_resv_add_fence(obj->base.resv, &rq->fence, > >> - DMA_RESV_USAGE_WRITE); > >> - i915_gem_object_set_moving_fence(obj, &rq->fence); > >> + DMA_RESV_USAGE_KERNEL); > >> i915_request_put(rq); > >> } > >> i915_gem_object_unlock(obj); > >> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > >> index 524477d8939e..d077f7b9eaad 100644 > >> --- a/drivers/gpu/drm/i915/i915_vma.c > >> +++ b/drivers/gpu/drm/i915/i915_vma.c > >> @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, > >> if (err) > >> return err; > >> > >> + if (vma->obj) { > >> + err = i915_gem_object_get_moving_fence(vma->obj, &moving); > >> + if (err) > >> + return err; > >> + } else { > >> + moving = NULL; > >> + } > >> + > >> if (flags & PIN_GLOBAL) > >> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); > >> > >> - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; > >> if (flags & vma->vm->bind_async_flags || moving) { > >> /* lock VM */ > >> err = i915_vm_lock_objects(vma->vm, ww); >
Am 08.04.22 um 11:33 schrieb Daniel Vetter: > On Fri, 8 Apr 2022 at 11:27, Christian König <christian.koenig@amd.com> wrote: >> Am 08.04.22 um 11:05 schrieb Jani Nikula: >>> On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: >>>> That should now be handled by the common dma_resv framework. >>>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: intel-gfx@lists.freedesktop.org >>> So, where are the i915 maintainer acks for merging this (and the other >>> patches in the series touching i915) via drm-misc-next? >>> >>> Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. >> I had the impression that it would be sufficient. >> >>> We don't merge i915 stuff without passing CI results. Apparently this >>> one failed enough machines that the CI had to be stopped entirely. >> That was unfortunately partially expected and pointed out by Matthew and >> Daniel before the push. > Uh I didn't realize that CI never tested this. Usually the way then is > to rebase onto drm-tip and figure out the merge story. Doing subsystem > wide changes while not on linux-next or drm-tip or another integration > tree is just fraught with surprises due to conflicts. > > Also "doesn't even compile" is really below the bar, and not the first > time this happened at all. And i915 isn't really an obscure driver > which is hard to compile test :-) Yeah, I'm really wondering how the build breakage slipped as well. I've double checked it multiple times now. My build system was certainly not complaining about anything, but right after the push both i915, VC4 and one of the core header changes broke the build on drm-tip. Going to try to figure out why that didn't worked as expected. Thanks, Christian > -Daniel > >> i915 for some reason extended the usage of the bo->moving fence despite >> the fact we had patches on the mailing list to entirely remove this feature. >> >> I couldn't get any sane CI results for weeks because of this and at some >> point we just had to go ahead and fix the clash in drm-tip. >> >> Sorry for any inconvenience cause by that. I hoped that we fixed all >> cases, but looks like we still missed some. >> >> Regards, >> Christian. >> >>> >>> BR, >>> Jani. >>> >>> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- >>>> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ >>>> .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- >>>> .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- >>>> drivers/gpu/drm/i915/i915_vma.c | 9 +++- >>>> 6 files changed, 21 insertions(+), 58 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> index 372bc220faeb..ffde7bc0a95d 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { >>>> /** >>>> * i915_gem_object_get_moving_fence - Get the object's moving fence if any >>>> * @obj: The object whose moving fence to get. >>>> + * @fence: The resulting fence >>>> * >>>> * A non-signaled moving fence means that there is an async operation >>>> * pending on the object that needs to be waited on before setting up >>>> * any GPU- or CPU PTEs to the object's pages. >>>> * >>>> - * Return: A refcounted pointer to the object's moving fence if any, >>>> - * NULL otherwise. >>>> + * Return: Negative error code or 0 for success. >>>> */ >>>> -struct dma_fence * >>>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) >>>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>>> + struct dma_fence **fence) >>>> { >>>> - return dma_fence_get(i915_gem_to_ttm(obj)->moving); >>>> -} >>>> - >>>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> - struct dma_fence *fence) >>>> -{ >>>> - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; >>>> - >>>> - if (*moving == fence) >>>> - return; >>>> - >>>> - dma_fence_put(*moving); >>>> - *moving = dma_fence_get(fence); >>>> + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, >>>> + fence); >>>> } >>>> >>>> /** >>>> @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>>> bool intr) >>>> { >>>> - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; >>>> - int ret; >>>> - >>>> assert_object_held(obj); >>>> - if (!fence) >>>> - return 0; >>>> - >>>> - ret = dma_fence_wait(fence, intr); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - if (fence->error) >>>> - return fence->error; >>>> - >>>> - i915_gem_to_ttm(obj)->moving = NULL; >>>> - dma_fence_put(fence); >>>> - return 0; >>>> + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, >>>> + intr, MAX_SCHEDULE_TIMEOUT); >>>> } >>>> >>>> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> index 02c37fe4a535..e11d82a9f7c3 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) >>>> i915_gem_object_unpin_pages(obj); >>>> } >>>> >>>> -struct dma_fence * >>>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); >>>> - >>>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> - struct dma_fence *fence); >>>> - >>>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>>> + struct dma_fence **fence); >>>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>>> bool intr); >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> index 438b8a95b3d1..a10716f4e717 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, >>>> return fence; >>>> } >>>> >>>> -static int >>>> -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, >>>> - struct i915_deps *deps) >>>> -{ >>>> - int ret; >>>> - >>>> - ret = i915_deps_add_dependency(deps, bo->moving, ctx); >>>> - if (!ret) >>>> - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); >>>> - >>>> - return ret; >>>> -} >>>> - >>>> /** >>>> * i915_ttm_move - The TTM move callback used by i915. >>>> * @bo: The buffer object. >>>> @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, >>>> struct i915_deps deps; >>>> >>>> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); >>>> - ret = prev_deps(bo, ctx, &deps); >>>> + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); >>>> if (ret) { >>>> i915_refct_sgt_put(dst_rsgt); >>>> return ret; >>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> index 4997ed18b6e4..0ad443a90c8b 100644 >>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, >>>> err = dma_resv_reserve_fences(obj->base.resv, 1); >>>> if (!err) >>>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>>> - DMA_RESV_USAGE_WRITE); >>>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>>> + DMA_RESV_USAGE_KERNEL); >>>> i915_request_put(rq); >>>> } >>>> if (err) >>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> index 3a6e3f6d239f..dfc34cc2ef8c 100644 >>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, >>>> i915_gem_object_unpin_pages(obj); >>>> if (rq) { >>>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>>> - DMA_RESV_USAGE_WRITE); >>>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>>> + DMA_RESV_USAGE_KERNEL); >>>> i915_request_put(rq); >>>> } >>>> i915_gem_object_unlock(obj); >>>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >>>> index 524477d8939e..d077f7b9eaad 100644 >>>> --- a/drivers/gpu/drm/i915/i915_vma.c >>>> +++ b/drivers/gpu/drm/i915/i915_vma.c >>>> @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, >>>> if (err) >>>> return err; >>>> >>>> + if (vma->obj) { >>>> + err = i915_gem_object_get_moving_fence(vma->obj, &moving); >>>> + if (err) >>>> + return err; >>>> + } else { >>>> + moving = NULL; >>>> + } >>>> + >>>> if (flags & PIN_GLOBAL) >>>> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); >>>> >>>> - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; >>>> if (flags & vma->vm->bind_async_flags || moving) { >>>> /* lock VM */ >>>> err = i915_vm_lock_objects(vma->vm, ww); >
On Fri, 08 Apr 2022, Christian König <christian.koenig@amd.com> wrote: > Am 08.04.22 um 11:05 schrieb Jani Nikula: >> On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: >>> That should now be handled by the common dma_resv framework. >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Cc: intel-gfx@lists.freedesktop.org >> So, where are the i915 maintainer acks for merging this (and the other >> patches in the series touching i915) via drm-misc-next? >> >> Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. > > I had the impression that it would be sufficient. Please don't assume. Please always ask for explicit acks from the maintainers before merging, and record the acks in the commit message. This has been standard policy for as long as I remember. Contrast with us merging non-trivial dma-buf changes via drm-intel-next with a Reviewed-by from someone who isn't a dma-buf maintainer, and not even bothering to Cc the maintainers. >> We don't merge i915 stuff without passing CI results. Apparently this >> one failed enough machines that the CI had to be stopped entirely. > > That was unfortunately partially expected and pointed out by Matthew and > Daniel before the push. > > i915 for some reason extended the usage of the bo->moving fence despite > the fact we had patches on the mailing list to entirely remove this feature. > > I couldn't get any sane CI results for weeks because of this and at some > point we just had to go ahead and fix the clash in drm-tip. Did you talk to the maintainers about it? BR, Jani. > > Sorry for any inconvenience cause by that. I hoped that we fixed all > cases, but looks like we still missed some. > > Regards, > Christian. > >> >> >> BR, >> Jani. >> >> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- >>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- >>> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ >>> .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- >>> .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- >>> drivers/gpu/drm/i915/i915_vma.c | 9 +++- >>> 6 files changed, 21 insertions(+), 58 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> index 372bc220faeb..ffde7bc0a95d 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>> @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { >>> /** >>> * i915_gem_object_get_moving_fence - Get the object's moving fence if any >>> * @obj: The object whose moving fence to get. >>> + * @fence: The resulting fence >>> * >>> * A non-signaled moving fence means that there is an async operation >>> * pending on the object that needs to be waited on before setting up >>> * any GPU- or CPU PTEs to the object's pages. >>> * >>> - * Return: A refcounted pointer to the object's moving fence if any, >>> - * NULL otherwise. >>> + * Return: Negative error code or 0 for success. >>> */ >>> -struct dma_fence * >>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) >>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>> + struct dma_fence **fence) >>> { >>> - return dma_fence_get(i915_gem_to_ttm(obj)->moving); >>> -} >>> - >>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>> - struct dma_fence *fence) >>> -{ >>> - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; >>> - >>> - if (*moving == fence) >>> - return; >>> - >>> - dma_fence_put(*moving); >>> - *moving = dma_fence_get(fence); >>> + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, >>> + fence); >>> } >>> >>> /** >>> @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>> bool intr) >>> { >>> - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; >>> - int ret; >>> - >>> assert_object_held(obj); >>> - if (!fence) >>> - return 0; >>> - >>> - ret = dma_fence_wait(fence, intr); >>> - if (ret) >>> - return ret; >>> - >>> - if (fence->error) >>> - return fence->error; >>> - >>> - i915_gem_to_ttm(obj)->moving = NULL; >>> - dma_fence_put(fence); >>> - return 0; >>> + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, >>> + intr, MAX_SCHEDULE_TIMEOUT); >>> } >>> >>> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> index 02c37fe4a535..e11d82a9f7c3 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>> @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) >>> i915_gem_object_unpin_pages(obj); >>> } >>> >>> -struct dma_fence * >>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); >>> - >>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>> - struct dma_fence *fence); >>> - >>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>> + struct dma_fence **fence); >>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>> bool intr); >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>> index 438b8a95b3d1..a10716f4e717 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>> @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, >>> return fence; >>> } >>> >>> -static int >>> -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, >>> - struct i915_deps *deps) >>> -{ >>> - int ret; >>> - >>> - ret = i915_deps_add_dependency(deps, bo->moving, ctx); >>> - if (!ret) >>> - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); >>> - >>> - return ret; >>> -} >>> - >>> /** >>> * i915_ttm_move - The TTM move callback used by i915. >>> * @bo: The buffer object. >>> @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, >>> struct i915_deps deps; >>> >>> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); >>> - ret = prev_deps(bo, ctx, &deps); >>> + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); >>> if (ret) { >>> i915_refct_sgt_put(dst_rsgt); >>> return ret; >>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>> index 4997ed18b6e4..0ad443a90c8b 100644 >>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>> @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, >>> err = dma_resv_reserve_fences(obj->base.resv, 1); >>> if (!err) >>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>> - DMA_RESV_USAGE_WRITE); >>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>> + DMA_RESV_USAGE_KERNEL); >>> i915_request_put(rq); >>> } >>> if (err) >>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>> index 3a6e3f6d239f..dfc34cc2ef8c 100644 >>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>> @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, >>> i915_gem_object_unpin_pages(obj); >>> if (rq) { >>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>> - DMA_RESV_USAGE_WRITE); >>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>> + DMA_RESV_USAGE_KERNEL); >>> i915_request_put(rq); >>> } >>> i915_gem_object_unlock(obj); >>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >>> index 524477d8939e..d077f7b9eaad 100644 >>> --- a/drivers/gpu/drm/i915/i915_vma.c >>> +++ b/drivers/gpu/drm/i915/i915_vma.c >>> @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, >>> if (err) >>> return err; >>> >>> + if (vma->obj) { >>> + err = i915_gem_object_get_moving_fence(vma->obj, &moving); >>> + if (err) >>> + return err; >>> + } else { >>> + moving = NULL; >>> + } >>> + >>> if (flags & PIN_GLOBAL) >>> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); >>> >>> - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; >>> if (flags & vma->vm->bind_async_flags || moving) { >>> /* lock VM */ >>> err = i915_vm_lock_objects(vma->vm, ww); >
Am 08.04.22 um 12:15 schrieb Jani Nikula: > On Fri, 08 Apr 2022, Christian König <christian.koenig@amd.com> wrote: >> Am 08.04.22 um 11:05 schrieb Jani Nikula: >>> On Thu, 07 Apr 2022, "Christian König" <ckoenig.leichtzumerken@gmail.com> wrote: >>>> That should now be handled by the common dma_resv framework. >>>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Cc: intel-gfx@lists.freedesktop.org >>> So, where are the i915 maintainer acks for merging this (and the other >>> patches in the series touching i915) via drm-misc-next? >>> >>> Daniel's Reviewed-by is not an ack to merge outside drm-intel-next. >> I had the impression that it would be sufficient. > Please don't assume. Please always ask for explicit acks from the > maintainers before merging, and record the acks in the commit > message. This has been standard policy for as long as I remember. Acked. > Contrast with us merging non-trivial dma-buf changes via drm-intel-next > with a Reviewed-by from someone who isn't a dma-buf maintainer, and not > even bothering to Cc the maintainers. Exactly that has happened before. And yes you are right we need to get more Acks here. > >>> We don't merge i915 stuff without passing CI results. Apparently this >>> one failed enough machines that the CI had to be stopped entirely. >> That was unfortunately partially expected and pointed out by Matthew and >> Daniel before the push. >> >> i915 for some reason extended the usage of the bo->moving fence despite >> the fact we had patches on the mailing list to entirely remove this feature. >> >> I couldn't get any sane CI results for weeks because of this and at some >> point we just had to go ahead and fix the clash in drm-tip. > Did you talk to the maintainers about it? Well, I noted merge problems on the list a few days before. I'm still puzzled why this didn't worked as expected. I've tested the drm-tip merge result on my build system before the push and it didn't showed anything broken. After the merge not just i915 broke, the whole core kernel didn't build because of a now missing include in futex.h. There must be something wrong with my setup here (or I just didn't had enough sleep). One major problem for me is that I can't run dim on my build system, but rather have to push/pull stuff from my laptop over SSH. My suspicion is that I was testing a stale checkout all the time because of this. Regards, Christian. > > > BR, > Jani. > >> Sorry for any inconvenience cause by that. I hoped that we fixed all >> cases, but looks like we still missed some. >> >> Regards, >> Christian. >> >>> >>> BR, >>> Jani. >>> >>> >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 41 ++++--------------- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.h | 8 +--- >>>> drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 15 +------ >>>> .../drm/i915/gem/selftests/i915_gem_migrate.c | 3 +- >>>> .../drm/i915/gem/selftests/i915_gem_mman.c | 3 +- >>>> drivers/gpu/drm/i915/i915_vma.c | 9 +++- >>>> 6 files changed, 21 insertions(+), 58 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> index 372bc220faeb..ffde7bc0a95d 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { >>>> /** >>>> * i915_gem_object_get_moving_fence - Get the object's moving fence if any >>>> * @obj: The object whose moving fence to get. >>>> + * @fence: The resulting fence >>>> * >>>> * A non-signaled moving fence means that there is an async operation >>>> * pending on the object that needs to be waited on before setting up >>>> * any GPU- or CPU PTEs to the object's pages. >>>> * >>>> - * Return: A refcounted pointer to the object's moving fence if any, >>>> - * NULL otherwise. >>>> + * Return: Negative error code or 0 for success. >>>> */ >>>> -struct dma_fence * >>>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) >>>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>>> + struct dma_fence **fence) >>>> { >>>> - return dma_fence_get(i915_gem_to_ttm(obj)->moving); >>>> -} >>>> - >>>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> - struct dma_fence *fence) >>>> -{ >>>> - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; >>>> - >>>> - if (*moving == fence) >>>> - return; >>>> - >>>> - dma_fence_put(*moving); >>>> - *moving = dma_fence_get(fence); >>>> + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, >>>> + fence); >>>> } >>>> >>>> /** >>>> @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>>> bool intr) >>>> { >>>> - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; >>>> - int ret; >>>> - >>>> assert_object_held(obj); >>>> - if (!fence) >>>> - return 0; >>>> - >>>> - ret = dma_fence_wait(fence, intr); >>>> - if (ret) >>>> - return ret; >>>> - >>>> - if (fence->error) >>>> - return fence->error; >>>> - >>>> - i915_gem_to_ttm(obj)->moving = NULL; >>>> - dma_fence_put(fence); >>>> - return 0; >>>> + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, >>>> + intr, MAX_SCHEDULE_TIMEOUT); >>>> } >>>> >>>> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> index 02c37fe4a535..e11d82a9f7c3 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h >>>> @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) >>>> i915_gem_object_unpin_pages(obj); >>>> } >>>> >>>> -struct dma_fence * >>>> -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); >>>> - >>>> -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, >>>> - struct dma_fence *fence); >>>> - >>>> +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, >>>> + struct dma_fence **fence); >>>> int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, >>>> bool intr); >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> index 438b8a95b3d1..a10716f4e717 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c >>>> @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, >>>> return fence; >>>> } >>>> >>>> -static int >>>> -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, >>>> - struct i915_deps *deps) >>>> -{ >>>> - int ret; >>>> - >>>> - ret = i915_deps_add_dependency(deps, bo->moving, ctx); >>>> - if (!ret) >>>> - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); >>>> - >>>> - return ret; >>>> -} >>>> - >>>> /** >>>> * i915_ttm_move - The TTM move callback used by i915. >>>> * @bo: The buffer object. >>>> @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, >>>> struct i915_deps deps; >>>> >>>> i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); >>>> - ret = prev_deps(bo, ctx, &deps); >>>> + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); >>>> if (ret) { >>>> i915_refct_sgt_put(dst_rsgt); >>>> return ret; >>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> index 4997ed18b6e4..0ad443a90c8b 100644 >>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c >>>> @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, >>>> err = dma_resv_reserve_fences(obj->base.resv, 1); >>>> if (!err) >>>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>>> - DMA_RESV_USAGE_WRITE); >>>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>>> + DMA_RESV_USAGE_KERNEL); >>>> i915_request_put(rq); >>>> } >>>> if (err) >>>> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> index 3a6e3f6d239f..dfc34cc2ef8c 100644 >>>> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c >>>> @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, >>>> i915_gem_object_unpin_pages(obj); >>>> if (rq) { >>>> dma_resv_add_fence(obj->base.resv, &rq->fence, >>>> - DMA_RESV_USAGE_WRITE); >>>> - i915_gem_object_set_moving_fence(obj, &rq->fence); >>>> + DMA_RESV_USAGE_KERNEL); >>>> i915_request_put(rq); >>>> } >>>> i915_gem_object_unlock(obj); >>>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >>>> index 524477d8939e..d077f7b9eaad 100644 >>>> --- a/drivers/gpu/drm/i915/i915_vma.c >>>> +++ b/drivers/gpu/drm/i915/i915_vma.c >>>> @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, >>>> if (err) >>>> return err; >>>> >>>> + if (vma->obj) { >>>> + err = i915_gem_object_get_moving_fence(vma->obj, &moving); >>>> + if (err) >>>> + return err; >>>> + } else { >>>> + moving = NULL; >>>> + } >>>> + >>>> if (flags & PIN_GLOBAL) >>>> wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); >>>> >>>> - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; >>>> if (flags & vma->vm->bind_async_flags || moving) { >>>> /* lock VM */ >>>> err = i915_vm_lock_objects(vma->vm, ww);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 372bc220faeb..ffde7bc0a95d 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -741,30 +741,19 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = { /** * i915_gem_object_get_moving_fence - Get the object's moving fence if any * @obj: The object whose moving fence to get. + * @fence: The resulting fence * * A non-signaled moving fence means that there is an async operation * pending on the object that needs to be waited on before setting up * any GPU- or CPU PTEs to the object's pages. * - * Return: A refcounted pointer to the object's moving fence if any, - * NULL otherwise. + * Return: Negative error code or 0 for success. */ -struct dma_fence * -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj) +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, + struct dma_fence **fence) { - return dma_fence_get(i915_gem_to_ttm(obj)->moving); -} - -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, - struct dma_fence *fence) -{ - struct dma_fence **moving = &i915_gem_to_ttm(obj)->moving; - - if (*moving == fence) - return; - - dma_fence_put(*moving); - *moving = dma_fence_get(fence); + return dma_resv_get_singleton(obj->base.resv, DMA_RESV_USAGE_KERNEL, + fence); } /** @@ -782,23 +771,9 @@ void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, bool intr) { - struct dma_fence *fence = i915_gem_to_ttm(obj)->moving; - int ret; - assert_object_held(obj); - if (!fence) - return 0; - - ret = dma_fence_wait(fence, intr); - if (ret) - return ret; - - if (fence->error) - return fence->error; - - i915_gem_to_ttm(obj)->moving = NULL; - dma_fence_put(fence); - return 0; + return dma_resv_wait_timeout(obj->base. resv, DMA_RESV_USAGE_KERNEL, + intr, MAX_SCHEDULE_TIMEOUT); } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 02c37fe4a535..e11d82a9f7c3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -520,12 +520,8 @@ i915_gem_object_finish_access(struct drm_i915_gem_object *obj) i915_gem_object_unpin_pages(obj); } -struct dma_fence * -i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj); - -void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj, - struct dma_fence *fence); - +int i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj, + struct dma_fence **fence); int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj, bool intr); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c index 438b8a95b3d1..a10716f4e717 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c @@ -467,19 +467,6 @@ __i915_ttm_move(struct ttm_buffer_object *bo, return fence; } -static int -prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx, - struct i915_deps *deps) -{ - int ret; - - ret = i915_deps_add_dependency(deps, bo->moving, ctx); - if (!ret) - ret = i915_deps_add_resv(deps, bo->base.resv, ctx); - - return ret; -} - /** * i915_ttm_move - The TTM move callback used by i915. * @bo: The buffer object. @@ -534,7 +521,7 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict, struct i915_deps deps; i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN); - ret = prev_deps(bo, ctx, &deps); + ret = i915_deps_add_resv(&deps, bo->base.resv, ctx); if (ret) { i915_refct_sgt_put(dst_rsgt); return ret; diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c index 4997ed18b6e4..0ad443a90c8b 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c @@ -219,8 +219,7 @@ static int __igt_lmem_pages_migrate(struct intel_gt *gt, err = dma_resv_reserve_fences(obj->base.resv, 1); if (!err) dma_resv_add_fence(obj->base.resv, &rq->fence, - DMA_RESV_USAGE_WRITE); - i915_gem_object_set_moving_fence(obj, &rq->fence); + DMA_RESV_USAGE_KERNEL); i915_request_put(rq); } if (err) diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c index 3a6e3f6d239f..dfc34cc2ef8c 100644 --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c @@ -1221,8 +1221,7 @@ static int __igt_mmap_migrate(struct intel_memory_region **placements, i915_gem_object_unpin_pages(obj); if (rq) { dma_resv_add_fence(obj->base.resv, &rq->fence, - DMA_RESV_USAGE_WRITE); - i915_gem_object_set_moving_fence(obj, &rq->fence); + DMA_RESV_USAGE_KERNEL); i915_request_put(rq); } i915_gem_object_unlock(obj); diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 524477d8939e..d077f7b9eaad 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1357,10 +1357,17 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, if (err) return err; + if (vma->obj) { + err = i915_gem_object_get_moving_fence(vma->obj, &moving); + if (err) + return err; + } else { + moving = NULL; + } + if (flags & PIN_GLOBAL) wakeref = intel_runtime_pm_get(&vma->vm->i915->runtime_pm); - moving = vma->obj ? i915_gem_object_get_moving_fence(vma->obj) : NULL; if (flags & vma->vm->bind_async_flags || moving) { /* lock VM */ err = i915_vm_lock_objects(vma->vm, ww);