| Message ID | 20200728062402.21942-1-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ttm/nouveau: consolidate slowpath reserve | expand |
Am 28.07.20 um 08:24 schrieb Dave Airlie: > From: Dave Airlie <airlied@redhat.com> > > The WARN_ON in the non-underscore path is off questionable value > (can we drop it from the non-slowpath?). At least for nouveau > where it's just looked up the gem object we know the ttm object > has a reference always so we can skip the check. Yeah, agreed. Wanted to look into removing that for quite some time as well. > It's probably nouveau could use execbut utils here at some point > but for now align the code between them to always call the __ > versions, and remove the non underscored version. Can we do it the other way around and remove all uses of the __ versions of the functions instead and then merge the __ version into the normal one without the WARN_ON()? Christian. > > Signed-off-by: Dave Airlie <airlied@redhat.com> > --- > drivers/gpu/drm/nouveau/nouveau_gem.c | 6 ++--- > drivers/gpu/drm/ttm/ttm_execbuf_util.c | 10 +-------- > include/drm/ttm/ttm_bo_driver.h | 31 +++++++++++--------------- > 3 files changed, 17 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c > index 81f111ad3f4f..d2d535d2ece1 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > @@ -422,15 +422,15 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv, > break; > } > > - ret = ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket); > + ret = __ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket); > if (ret) { > list_splice_tail_init(&vram_list, &op->list); > list_splice_tail_init(&gart_list, &op->list); > list_splice_tail_init(&both_list, &op->list); > validate_fini_no_ticket(op, chan, NULL, NULL); > if (unlikely(ret == -EDEADLK)) { > - ret = ttm_bo_reserve_slowpath(&nvbo->bo, true, > - &op->ticket); > + ret = __ttm_bo_reserve_slowpath(&nvbo->bo, true, > + &op->ticket); > if (!ret) > res_bo = nvbo; > } > diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c > index 1797f04c0534..a24f13bc90fb 100644 > --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c > +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c > @@ -119,13 +119,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, > ttm_eu_backoff_reservation_reverse(list, entry); > > if (ret == -EDEADLK) { > - if (intr) { > - ret = dma_resv_lock_slow_interruptible(bo->base.resv, > - ticket); > - } else { > - dma_resv_lock_slow(bo->base.resv, ticket); > - ret = 0; > - } > + ret = __ttm_bo_reserve_slowpath(bo, intr, ticket); > } > > if (!ret && entry->num_shared) > @@ -133,8 +127,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, > entry->num_shared); > > if (unlikely(ret != 0)) { > - if (ret == -EINTR) > - ret = -ERESTARTSYS; > if (ticket) { > ww_acquire_done(ticket); > ww_acquire_fini(ticket); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index 5a37f1cc057e..563b970949a1 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -695,7 +695,7 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo, > } > > /** > - * ttm_bo_reserve_slowpath: > + * __ttm_bo_reserve_slowpath: > * @bo: A pointer to a struct ttm_buffer_object. > * @interruptible: Sleep interruptible if waiting. > * @sequence: Set (@bo)->sequence to this value after lock > @@ -704,24 +704,19 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo, > * from all our other reservations. Because there are no other reservations > * held by us, this function cannot deadlock any more. > */ > -static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, > - bool interruptible, > - struct ww_acquire_ctx *ticket) > +static inline int __ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, > + bool interruptible, > + struct ww_acquire_ctx *ticket) > { > - int ret = 0; > - > - WARN_ON(!kref_read(&bo->kref)); > - > - if (interruptible) > - ret = dma_resv_lock_slow_interruptible(bo->base.resv, > - ticket); > - else > - dma_resv_lock_slow(bo->base.resv, ticket); > - > - if (ret == -EINTR) > - ret = -ERESTARTSYS; > - > - return ret; > + if (interruptible) { > + int ret = dma_resv_lock_slow_interruptible(bo->base.resv, > + ticket); > + if (ret == -EINTR) > + ret = -ERESTARTSYS; > + return ret; > + } > + dma_resv_lock_slow(bo->base.resv, ticket); > + return 0; > } > > /**
On Tue, 28 Jul 2020 at 17:30, Christian König <christian.koenig@amd.com> wrote: > > Am 28.07.20 um 08:24 schrieb Dave Airlie: > > From: Dave Airlie <airlied@redhat.com> > > > > The WARN_ON in the non-underscore path is off questionable value > > (can we drop it from the non-slowpath?). At least for nouveau > > where it's just looked up the gem object we know the ttm object > > has a reference always so we can skip the check. > > Yeah, agreed. Wanted to look into removing that for quite some time as well. > > > It's probably nouveau could use execbut utils here at some point > > but for now align the code between them to always call the __ > > versions, and remove the non underscored version. > > Can we do it the other way around and remove all uses of the __ versions > of the functions instead and then merge the __ version into the normal > one without the WARN_ON()? Yes sounds like a plan, I just wasn't sure the WARN_ON had value, bit since you agree is dubious at best, I'm happy to rip it out. Will send a v2 tomorrow. Dave.
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index 81f111ad3f4f..d2d535d2ece1 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -422,15 +422,15 @@ validate_init(struct nouveau_channel *chan, struct drm_file *file_priv, break; } - ret = ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket); + ret = __ttm_bo_reserve(&nvbo->bo, true, false, &op->ticket); if (ret) { list_splice_tail_init(&vram_list, &op->list); list_splice_tail_init(&gart_list, &op->list); list_splice_tail_init(&both_list, &op->list); validate_fini_no_ticket(op, chan, NULL, NULL); if (unlikely(ret == -EDEADLK)) { - ret = ttm_bo_reserve_slowpath(&nvbo->bo, true, - &op->ticket); + ret = __ttm_bo_reserve_slowpath(&nvbo->bo, true, + &op->ticket); if (!ret) res_bo = nvbo; } diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index 1797f04c0534..a24f13bc90fb 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -119,13 +119,7 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, ttm_eu_backoff_reservation_reverse(list, entry); if (ret == -EDEADLK) { - if (intr) { - ret = dma_resv_lock_slow_interruptible(bo->base.resv, - ticket); - } else { - dma_resv_lock_slow(bo->base.resv, ticket); - ret = 0; - } + ret = __ttm_bo_reserve_slowpath(bo, intr, ticket); } if (!ret && entry->num_shared) @@ -133,8 +127,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, entry->num_shared); if (unlikely(ret != 0)) { - if (ret == -EINTR) - ret = -ERESTARTSYS; if (ticket) { ww_acquire_done(ticket); ww_acquire_fini(ticket); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 5a37f1cc057e..563b970949a1 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -695,7 +695,7 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo, } /** - * ttm_bo_reserve_slowpath: + * __ttm_bo_reserve_slowpath: * @bo: A pointer to a struct ttm_buffer_object. * @interruptible: Sleep interruptible if waiting. * @sequence: Set (@bo)->sequence to this value after lock @@ -704,24 +704,19 @@ static inline int ttm_bo_reserve(struct ttm_buffer_object *bo, * from all our other reservations. Because there are no other reservations * held by us, this function cannot deadlock any more. */ -static inline int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, - bool interruptible, - struct ww_acquire_ctx *ticket) +static inline int __ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, + bool interruptible, + struct ww_acquire_ctx *ticket) { - int ret = 0; - - WARN_ON(!kref_read(&bo->kref)); - - if (interruptible) - ret = dma_resv_lock_slow_interruptible(bo->base.resv, - ticket); - else - dma_resv_lock_slow(bo->base.resv, ticket); - - if (ret == -EINTR) - ret = -ERESTARTSYS; - - return ret; + if (interruptible) { + int ret = dma_resv_lock_slow_interruptible(bo->base.resv, + ticket); + if (ret == -EINTR) + ret = -ERESTARTSYS; + return ret; + } + dma_resv_lock_slow(bo->base.resv, ticket); + return 0; } /**