| Message ID | 20200924051845.397177-31-airlied@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TTM move refactoring | expand |
On Thu, Sep 24, 2020 at 03:18:30PM +1000, Dave Airlie wrote: > From: Dave Airlie <airlied@redhat.com> > > Signed-off-by: Dave Airlie <airlied@redhat.com> A bikeshed, but why not just call this ->invalidate? ->move_notify needed the _notify because we already had the ->move callback, but there's not invalidate that also needs a invalidate_notify. And a callback kinda implies that the driver gets notified about some stuff, but doesn't really add anything about what it should do now. Plus if we go with notify, I guess it should be called ->delete_notify, and use that to do it's invalidation. </bikeshed> Otherwise I think this entire series is a solid demidlayer of all the move stuff here. Cheers, Daniel > --- > drivers/gpu/drm/ttm/ttm_bo.c | 4 +++- > include/drm/ttm/ttm_bo_driver.h | 7 +++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index a2a61a8d1394..ba69c682e53b 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -282,7 +282,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > > static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > { > - if (bo->bdev->driver->move_notify) > + if (bo->bdev->driver->invalidate_notify) > + bo->bdev->driver->invalidate_notify(bo); > + else if (bo->bdev->driver->move_notify) > bo->bdev->driver->move_notify(bo, false, NULL); > > ttm_bo_tt_destroy(bo); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index cfb151dbb2d0..da4afe669664 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -165,6 +165,13 @@ struct ttm_bo_driver { > void (*move_notify)(struct ttm_buffer_object *bo, > bool evict, > struct ttm_resource *new_mem); > + > + /** > + * Hook to notify driver about a bo being torn down. > + * can be used for invalidation instead of move_notify. > + */ > + void (*invalidate_notify)(struct ttm_buffer_object *bo); > + > /* notify the driver we are taking a fault on this BO > * and have reserved it */ > int (*fault_reserve_notify)(struct ttm_buffer_object *bo); > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 24.09.20 um 07:18 schrieb Dave Airlie: > From: Dave Airlie <airlied@redhat.com> > > Signed-off-by: Dave Airlie <airlied@redhat.com> NAK, completely unnecessary. We should rather do the remaining accounting in the already existing release_notify() callback. That makes much more sense and if I'm not completely mistaken could actually fix a bug in amdgpu. Christian. > --- > drivers/gpu/drm/ttm/ttm_bo.c | 4 +++- > include/drm/ttm/ttm_bo_driver.h | 7 +++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index a2a61a8d1394..ba69c682e53b 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -282,7 +282,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, > > static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) > { > - if (bo->bdev->driver->move_notify) > + if (bo->bdev->driver->invalidate_notify) > + bo->bdev->driver->invalidate_notify(bo); > + else if (bo->bdev->driver->move_notify) > bo->bdev->driver->move_notify(bo, false, NULL); > > ttm_bo_tt_destroy(bo); > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h > index cfb151dbb2d0..da4afe669664 100644 > --- a/include/drm/ttm/ttm_bo_driver.h > +++ b/include/drm/ttm/ttm_bo_driver.h > @@ -165,6 +165,13 @@ struct ttm_bo_driver { > void (*move_notify)(struct ttm_buffer_object *bo, > bool evict, > struct ttm_resource *new_mem); > + > + /** > + * Hook to notify driver about a bo being torn down. > + * can be used for invalidation instead of move_notify. > + */ > + void (*invalidate_notify)(struct ttm_buffer_object *bo); > + > /* notify the driver we are taking a fault on this BO > * and have reserved it */ > int (*fault_reserve_notify)(struct ttm_buffer_object *bo);
On Thu, 24 Sep 2020 at 22:25, Christian König <christian.koenig@amd.com> wrote: > > Am 24.09.20 um 07:18 schrieb Dave Airlie: > > From: Dave Airlie <airlied@redhat.com> > > > > Signed-off-by: Dave Airlie <airlied@redhat.com> > > NAK, completely unnecessary. > > We should rather do the remaining accounting in the already existing > release_notify() callback. > > That makes much more sense and if I'm not completely mistaken could > actually fix a bug in amdgpu. release_notify is called from one path, but there is a path in eviction where if it gets pass 0 placements it tears down the memory, and allocates a tt. I'm considering whether it should be acceptable to call evict with no placements (though maybe that just means evict to system). Dave.
Am 29.09.20 um 05:23 schrieb Dave Airlie: > On Thu, 24 Sep 2020 at 22:25, Christian König <christian.koenig@amd.com> wrote: >> Am 24.09.20 um 07:18 schrieb Dave Airlie: >>> From: Dave Airlie <airlied@redhat.com> >>> >>> Signed-off-by: Dave Airlie <airlied@redhat.com> >> NAK, completely unnecessary. >> >> We should rather do the remaining accounting in the already existing >> release_notify() callback. >> >> That makes much more sense and if I'm not completely mistaken could >> actually fix a bug in amdgpu. > release_notify is called from one path, but there is a path in > eviction where if it gets pass 0 placements > it tears down the memory, and allocates a tt. You mean for the pipelined gutting? Mhm, I see. Probably better to call the move callback for those cases as well. Ok, go ahead with that approach for now. But we really need to clean that up further. Christian. > > I'm considering whether it should be acceptable to call evict with no > placements (though maybe that just means evict to system). > > Dave.
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index a2a61a8d1394..ba69c682e53b 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -282,7 +282,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo, static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) { - if (bo->bdev->driver->move_notify) + if (bo->bdev->driver->invalidate_notify) + bo->bdev->driver->invalidate_notify(bo); + else if (bo->bdev->driver->move_notify) bo->bdev->driver->move_notify(bo, false, NULL); ttm_bo_tt_destroy(bo); diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index cfb151dbb2d0..da4afe669664 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -165,6 +165,13 @@ struct ttm_bo_driver { void (*move_notify)(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *new_mem); + + /** + * Hook to notify driver about a bo being torn down. + * can be used for invalidation instead of move_notify. + */ + void (*invalidate_notify)(struct ttm_buffer_object *bo); + /* notify the driver we are taking a fault on this BO * and have reserved it */ int (*fault_reserve_notify)(struct ttm_buffer_object *bo);