diff mbox series

[30/45] drm/ttm: add a new invalidate notify callback.

Message ID 20200924051845.397177-31-airlied@gmail.com
State New, archived
Headers show
Series TTM move refactoring | expand

Commit Message

Dave Airlie Sept. 24, 2020, 5:18 a.m. UTC
From: Dave Airlie <airlied@redhat.com>

Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c    | 4 +++-
 include/drm/ttm/ttm_bo_driver.h | 7 +++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Sept. 24, 2020, 9:33 a.m. UTC | #1
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
Christian König Sept. 24, 2020, 12:25 p.m. UTC | #2
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);
Dave Airlie Sept. 29, 2020, 3:23 a.m. UTC | #3
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.
Christian König Sept. 29, 2020, 7:05 a.m. UTC | #4
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 mbox series

Patch

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);