| Message ID | 1352728811-21860-2-git-send-email-maarten.lankhorst@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: > move to release_list instead Can you describe why this change is made? cleanup? reorder locks in a later patch? Also please describe why you need move_notify and ttm unbind / destroy to be outside of reservation, because that's the main change in this patch and it's not even mentioned in the commit message. Thanks, Thomas > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 47 +++++++++++++------------------------------- > 1 file changed, 14 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index 9c48e8f..74d6e7c 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -143,12 +143,20 @@ static void ttm_bo_release_list(struct kref *list_kref) > BUG_ON(atomic_read(&bo->kref.refcount)); > BUG_ON(atomic_read(&bo->cpu_writers)); > BUG_ON(bo->sync_obj != NULL); > - BUG_ON(bo->mem.mm_node != NULL); > BUG_ON(!list_empty(&bo->lru)); > BUG_ON(!list_empty(&bo->ddestroy)); > > - if (bo->ttm) > + if (bo->bdev->driver->move_notify) > + bo->bdev->driver->move_notify(bo, NULL); > + > + if (bo->ttm) { > + ttm_tt_unbind(bo->ttm); > ttm_tt_destroy(bo->ttm); > + bo->ttm = NULL; > + } > + ttm_bo_mem_put(bo, &bo->mem); > + BUG_ON(bo->mem.mm_node != NULL); > + > atomic_dec(&bo->glob->bo_count); > if (bo->destroy) > bo->destroy(bo); > @@ -466,35 +474,6 @@ out_err: > return ret; > } > > -/** > - * Call bo::reserved. > - * Will release GPU memory type usage on destruction. > - * This is the place to put in driver specific hooks to release > - * driver private resources. > - * Will release the bo::reserved lock. > - */ > - > -static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > -{ > - if (bo->bdev->driver->move_notify) > - bo->bdev->driver->move_notify(bo, NULL); > - > - if (bo->ttm) { > - ttm_tt_unbind(bo->ttm); > - ttm_tt_destroy(bo->ttm); > - bo->ttm = NULL; > - } > - ttm_bo_mem_put(bo, &bo->mem); > - > - atomic_set(&bo->reserved, 0); > - > - /* > - * Make processes trying to reserve really pick it up. > - */ > - smp_mb__after_atomic_dec(); > - wake_up_all(&bo->event_queue); > -} > - > static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) > { > struct ttm_bo_device *bdev = bo->bdev; > @@ -523,8 +502,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) > spin_unlock(&bdev->fence_lock); > put_count = ttm_bo_del_from_lru(bo); > > + atomic_set(&bo->reserved, 0); > + wake_up_all(&bo->event_queue); > spin_unlock(&glob->lru_lock); > - ttm_bo_cleanup_memtype_use(bo); > > ttm_bo_list_ref_sub(bo, put_count, true); > > @@ -619,8 +599,9 @@ retry_reserve: > list_del_init(&bo->ddestroy); > ++put_count; > > + atomic_set(&bo->reserved, 0); > + wake_up_all(&bo->event_queue); > spin_unlock(&glob->lru_lock); > - ttm_bo_cleanup_memtype_use(bo); > > ttm_bo_list_ref_sub(bo, put_count, true); >
Op 19-11-12 14:26, Thomas Hellstrom schreef: > Hi, > > On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: >> move to release_list instead > > Can you describe why this change is made? cleanup? reorder locks in a later patch? > Also please describe why you need move_notify and ttm unbind / destroy to be outside of > reservation, because that's the main change in this patch and it's not even mentioned in the > commit message. Ok is a reword enough? In that case I'll resend. I moved all the destruction to happen when release_list refcount drops to 0. This removes the special handling of ttm_bo_cleanup_memtype_use, and makes it part of the normal bo destruction instead. It also meant that move_notify and unbind/destroy was without reservation, simply because it was done during normal destruction instead. At that point you may no longer hold a reservation, but you can already be sure you're the only one touching it. It is optional and I can drop this patch if the behavior change is unwanted. ~Maarten
On 11/19/2012 03:03 PM, Maarten Lankhorst wrote: > Op 19-11-12 14:26, Thomas Hellstrom schreef: >> Hi, >> >> On 11/12/2012 03:00 PM, Maarten Lankhorst wrote: >>> move to release_list instead >> Can you describe why this change is made? cleanup? reorder locks in a later patch? >> Also please describe why you need move_notify and ttm unbind / destroy to be outside of >> reservation, because that's the main change in this patch and it's not even mentioned in the >> commit message. > Ok is a reword enough? In that case I'll resend. > > I moved all the destruction to happen when release_list refcount drops to 0. > This removes the special handling of ttm_bo_cleanup_memtype_use, and > makes it part of the normal bo destruction instead. > > It also meant that move_notify and unbind/destroy was without reservation, simply > because it was done during normal destruction instead. At that point you may no longer > hold a reservation, but you can already be sure you're the only one touching it. That's true, but even if we are the only users it would be good to keep reservation when calling move_notify and the unbind stuff, simply because drivers can easily detect reservation bugs if these functions are always called reserved. if we kan keep the reservation here without too much trouble, I'd like us to do so. Thanks, Thomas
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index 9c48e8f..74d6e7c 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -143,12 +143,20 @@ static void ttm_bo_release_list(struct kref *list_kref) BUG_ON(atomic_read(&bo->kref.refcount)); BUG_ON(atomic_read(&bo->cpu_writers)); BUG_ON(bo->sync_obj != NULL); - BUG_ON(bo->mem.mm_node != NULL); BUG_ON(!list_empty(&bo->lru)); BUG_ON(!list_empty(&bo->ddestroy)); - if (bo->ttm) + if (bo->bdev->driver->move_notify) + bo->bdev->driver->move_notify(bo, NULL); + + if (bo->ttm) { + ttm_tt_unbind(bo->ttm); ttm_tt_destroy(bo->ttm); + bo->ttm = NULL; + } + ttm_bo_mem_put(bo, &bo->mem); + BUG_ON(bo->mem.mm_node != NULL); + atomic_dec(&bo->glob->bo_count); if (bo->destroy) bo->destroy(bo); @@ -466,35 +474,6 @@ out_err: return ret; } -/** - * Call bo::reserved. - * Will release GPU memory type usage on destruction. - * This is the place to put in driver specific hooks to release - * driver private resources. - * Will release the bo::reserved lock. - */ - -static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) -{ - if (bo->bdev->driver->move_notify) - bo->bdev->driver->move_notify(bo, NULL); - - if (bo->ttm) { - ttm_tt_unbind(bo->ttm); - ttm_tt_destroy(bo->ttm); - bo->ttm = NULL; - } - ttm_bo_mem_put(bo, &bo->mem); - - atomic_set(&bo->reserved, 0); - - /* - * Make processes trying to reserve really pick it up. - */ - smp_mb__after_atomic_dec(); - wake_up_all(&bo->event_queue); -} - static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) { struct ttm_bo_device *bdev = bo->bdev; @@ -523,8 +502,9 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) spin_unlock(&bdev->fence_lock); put_count = ttm_bo_del_from_lru(bo); + atomic_set(&bo->reserved, 0); + wake_up_all(&bo->event_queue); spin_unlock(&glob->lru_lock); - ttm_bo_cleanup_memtype_use(bo); ttm_bo_list_ref_sub(bo, put_count, true); @@ -619,8 +599,9 @@ retry_reserve: list_del_init(&bo->ddestroy); ++put_count; + atomic_set(&bo->reserved, 0); + wake_up_all(&bo->event_queue); spin_unlock(&glob->lru_lock); - ttm_bo_cleanup_memtype_use(bo); ttm_bo_list_ref_sub(bo, put_count, true);
move to release_list instead Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> --- drivers/gpu/drm/ttm/ttm_bo.c | 47 +++++++++++++------------------------------- 1 file changed, 14 insertions(+), 33 deletions(-)