[PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Christian König]

Swapping bo->mem was completely unecessary. Cleanup the function which
is just a leftover from a TTM cleanup.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
index a0992f0b8afd..0c2233ee6029 100644
--- a/drivers/gpu/drm/drm_gem_vram_helper.c
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
 	*pl = gbo->placement;
 }
 
-static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
-					       bool evict,
-					       struct ttm_resource *new_mem)
+static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo)
 {
 	struct ttm_buffer_object *bo = &gbo->bo;
 	struct drm_device *dev = bo->base.dev;
@@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo,
 				       struct ttm_operation_ctx *ctx,
 				       struct ttm_resource *new_mem)
 {
-	int ret;
-
-	drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
-	ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
-	if (ret) {
-		swap(*new_mem, gbo->bo.mem);
-		drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
-		swap(*new_mem, gbo->bo.mem);
-	}
-	return ret;
+	drm_gem_vram_bo_driver_move_notify(gbo);
+	return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
 }
 
 /*
@@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
 
 	gbo = drm_gem_vram_of_bo(bo);
 
-	drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
+	drm_gem_vram_bo_driver_move_notify(gbo);
 }
 
 static int bo_driver_move(struct ttm_buffer_object *bo,
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/qxl: clean up qxl_bo_move_notify

[Christian König]

Remove the unused evict parameter and drop swapping bo->mem.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/qxl/qxl_ttm.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index b7f77eb685cb..47afe95d04a1 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -121,7 +121,6 @@ static struct ttm_tt *qxl_ttm_tt_create(struct ttm_buffer_object *bo,
 }
 
 static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
-			       bool evict,
 			       struct ttm_resource *new_mem)
 {
 	struct qxl_bo *qbo;
@@ -144,29 +143,22 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
 	struct ttm_resource *old_mem = &bo->mem;
 	int ret;
 
-	qxl_bo_move_notify(bo, evict, new_mem);
+	qxl_bo_move_notify(bo, new_mem);
 
 	ret = ttm_bo_wait_ctx(bo, ctx);
 	if (ret)
-		goto out;
+		return ret;
 
 	if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
 		ttm_bo_move_null(bo, new_mem);
 		return 0;
 	}
-	ret = ttm_bo_move_memcpy(bo, ctx, new_mem);
-out:
-	if (ret) {
-		swap(*new_mem, bo->mem);
-		qxl_bo_move_notify(bo, false, new_mem);
-		swap(*new_mem, bo->mem);
-	}
-	return ret;
+	return ttm_bo_move_memcpy(bo, ctx, new_mem);
 }
 
 static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo)
 {
-	qxl_bo_move_notify(bo, false, NULL);
+	qxl_bo_move_notify(bo, NULL);
 }
 
 static struct ttm_device_funcs qxl_bo_driver = {
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/nouveau: clean up nouveau_bo_move_ntfy

[Christian König]

Just another leftover from a TTM cleanup.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index ca2a8ae1938e..9bb8cee3df40 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -861,9 +861,8 @@ nouveau_bo_move_init(struct nouveau_drm *drm)
 	NV_INFO(drm, "MM: using %s for buffer copies\n", name);
 }
 
-static void
-nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict,
-		     struct ttm_resource *new_reg)
+static void nouveau_bo_move_ntfy(struct ttm_buffer_object *bo,
+				 struct ttm_resource *new_reg)
 {
 	struct nouveau_mem *mem = new_reg ? nouveau_mem(new_reg) : NULL;
 	struct nouveau_bo *nvbo = nouveau_bo(bo);
@@ -949,7 +948,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 			return ret;
 	}
 
-	nouveau_bo_move_ntfy(bo, evict, new_reg);
+	nouveau_bo_move_ntfy(bo, new_reg);
 	ret = ttm_bo_wait_ctx(bo, ctx);
 	if (ret)
 		goto out_ntfy;
@@ -1014,9 +1013,7 @@ nouveau_bo_move(struct ttm_buffer_object *bo, bool evict,
 	}
 out_ntfy:
 	if (ret) {
-		swap(*new_reg, bo->mem);
-		nouveau_bo_move_ntfy(bo, false, new_reg);
-		swap(*new_reg, bo->mem);
+		nouveau_bo_move_ntfy(bo, &bo->mem);
 	}
 	return ret;
 }
@@ -1290,7 +1287,7 @@ nouveau_bo_fence(struct nouveau_bo *nvbo, struct nouveau_fence *fence, bool excl
 static void
 nouveau_bo_delete_mem_notify(struct ttm_buffer_object *bo)
 {
-	nouveau_bo_move_ntfy(bo, false, NULL);
+	nouveau_bo_move_ntfy(bo, NULL);
 }
 
 struct ttm_device_funcs nouveau_bo_driver = {
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/vmwgfx: clean up vmw_move_notify

[Christian König]

Instead of swapping bo->mem just give old and new as parameters.

Also drop unused parameters and code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  3 ++-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |  9 +++++---
 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 26 +++++++---------------
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index e65b00f8336d..21a72fa7b9fd 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -775,7 +775,8 @@ extern void vmw_resource_unreserve(struct vmw_resource *res,
 				   struct vmw_buffer_object *new_backup,
 				   unsigned long new_backup_offset);
 extern void vmw_query_move_notify(struct ttm_buffer_object *bo,
-				  struct ttm_resource *mem);
+				  struct ttm_resource *old_mem,
+				  struct ttm_resource *new_mem);
 extern int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob);
 extern void vmw_resource_evict_all(struct vmw_private *dev_priv);
 extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index c3a724e37104..35f02958ee2c 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -847,13 +847,15 @@ int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob)
  * vmw_query_move_notify - Read back cached query states
  *
  * @bo: The TTM buffer object about to move.
- * @mem: The memory region @bo is moving to.
+ * @old_mem: The memory region @bo is moving from.
+ * @new_mem: The memory region @bo is moving to.
  *
  * Called before the query MOB is swapped out to read back cached query
  * states from the device.
  */
 void vmw_query_move_notify(struct ttm_buffer_object *bo,
-			   struct ttm_resource *mem)
+			   struct ttm_resource *old_mem,
+			   struct ttm_resource *new_mem)
 {
 	struct vmw_buffer_object *dx_query_mob;
 	struct ttm_device *bdev = bo->bdev;
@@ -871,7 +873,8 @@ void vmw_query_move_notify(struct ttm_buffer_object *bo,
 	}
 
 	/* If BO is being moved from MOB to system memory */
-	if (mem->mem_type == TTM_PL_SYSTEM && bo->mem.mem_type == VMW_PL_MOB) {
+	if (new_mem->mem_type == TTM_PL_SYSTEM &&
+	    old_mem->mem_type == VMW_PL_MOB) {
 		struct vmw_fence_obj *fence;
 
 		(void) vmw_query_readback_all(dx_query_mob);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
index 63f10c865061..38801462134e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
@@ -692,20 +692,19 @@ static int vmw_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *
  *
  * @bo: The TTM buffer object about to move.
  * @evict: Unused
- * @mem: The struct ttm_resource indicating to what memory
+ * @old_mem: The old memory where we move from
+ * @new_mem: The struct ttm_resource indicating to what memory
  *       region the move is taking place.
  *
  * Calls move_notify for all subsystems needing it.
  * (currently only resources).
  */
 static void vmw_move_notify(struct ttm_buffer_object *bo,
-			    bool evict,
-			    struct ttm_resource *mem)
+			    struct ttm_resource *old_mem,
+			    struct ttm_resource *new_mem)
 {
-	if (!mem)
-		return;
-	vmw_bo_move_notify(bo, mem);
-	vmw_query_move_notify(bo, mem);
+	vmw_bo_move_notify(bo, new_mem);
+	vmw_query_move_notify(bo, old_mem, new_mem);
 }
 
 
@@ -736,7 +735,7 @@ static int vmw_move(struct ttm_buffer_object *bo,
 			return ret;
 	}
 
-	vmw_move_notify(bo, evict, new_mem);
+	vmw_move_notify(bo, &bo->mem, new_mem);
 
 	if (old_man->use_tt && new_man->use_tt) {
 		if (bo->mem.mem_type == TTM_PL_SYSTEM) {
@@ -758,18 +757,10 @@ static int vmw_move(struct ttm_buffer_object *bo,
 	}
 	return 0;
 fail:
-	swap(*new_mem, bo->mem);
-	vmw_move_notify(bo, false, new_mem);
-	swap(*new_mem, bo->mem);
+	vmw_move_notify(bo, new_mem, &bo->mem);
 	return ret;
 }
 
-static void
-vmw_delete_mem_notify(struct ttm_buffer_object *bo)
-{
-	vmw_move_notify(bo, false, NULL);
-}
-
 struct ttm_device_funcs vmw_bo_driver = {
 	.ttm_tt_create = &vmw_ttm_tt_create,
 	.ttm_tt_populate = &vmw_ttm_populate,
@@ -779,7 +770,6 @@ struct ttm_device_funcs vmw_bo_driver = {
 	.evict_flags = vmw_evict_flags,
 	.move = vmw_move,
 	.verify_access = vmw_verify_access,
-	.delete_mem_notify = vmw_delete_mem_notify,
 	.swap_notify = vmw_swap_notify,
 	.io_mem_reserve = &vmw_ttm_io_mem_reserve,
 };
-- 
2.25.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2571 bytes --]

Hi

Am 11.02.21 um 14:16 schrieb Christian König:
> Swapping bo->mem was completely unecessary. Cleanup the function which
> is just a leftover from a TTM cleanup.

Yes this was introduced in a recent cleanup effort. Can you explain what 
the code intends to do? It seems as if it tries to "re-unmap the BO" if 
the move_memcpy fails.

If the move_memcpy fails now, it seems like we can live without 
reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)

Best regards
Thomas

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
>   1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index a0992f0b8afd..0c2233ee6029 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
>   	*pl = gbo->placement;
>   }
>   
> -static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
> -					       bool evict,
> -					       struct ttm_resource *new_mem)
> +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo)
>   {
>   	struct ttm_buffer_object *bo = &gbo->bo;
>   	struct drm_device *dev = bo->base.dev;
> @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo,
>   				       struct ttm_operation_ctx *ctx,
>   				       struct ttm_resource *new_mem)
>   {
> -	int ret;
> -
> -	drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
> -	ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
> -	if (ret) {
> -		swap(*new_mem, gbo->bo.mem);
> -		drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
> -		swap(*new_mem, gbo->bo.mem);
> -	}
> -	return ret;
> +	drm_gem_vram_bo_driver_move_notify(gbo);
> +	return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>   }
>   
>   /*
> @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
>   
>   	gbo = drm_gem_vram_of_bo(bo);
>   
> -	drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
> +	drm_gem_vram_bo_driver_move_notify(gbo);
>   }
>   
>   static int bo_driver_move(struct ttm_buffer_object *bo,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/qxl: clean up qxl_bo_move_notify

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2035 bytes --]

Hi

Am 11.02.21 um 14:16 schrieb Christian König:
> Remove the unused evict parameter and drop swapping bo->mem.

Could you provide the same patch for vram-helpers as well?

Best regards
Thomas

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/qxl/qxl_ttm.c | 16 ++++------------
>   1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
> index b7f77eb685cb..47afe95d04a1 100644
> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> @@ -121,7 +121,6 @@ static struct ttm_tt *qxl_ttm_tt_create(struct ttm_buffer_object *bo,
>   }
>   
>   static void qxl_bo_move_notify(struct ttm_buffer_object *bo,
> -			       bool evict,
>   			       struct ttm_resource *new_mem)
>   {
>   	struct qxl_bo *qbo;
> @@ -144,29 +143,22 @@ static int qxl_bo_move(struct ttm_buffer_object *bo, bool evict,
>   	struct ttm_resource *old_mem = &bo->mem;
>   	int ret;
>   
> -	qxl_bo_move_notify(bo, evict, new_mem);
> +	qxl_bo_move_notify(bo, new_mem);
>   
>   	ret = ttm_bo_wait_ctx(bo, ctx);
>   	if (ret)
> -		goto out;
> +		return ret;
>   
>   	if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
>   		ttm_bo_move_null(bo, new_mem);
>   		return 0;
>   	}
> -	ret = ttm_bo_move_memcpy(bo, ctx, new_mem);
> -out:
> -	if (ret) {
> -		swap(*new_mem, bo->mem);
> -		qxl_bo_move_notify(bo, false, new_mem);
> -		swap(*new_mem, bo->mem);
> -	}
> -	return ret;
> +	return ttm_bo_move_memcpy(bo, ctx, new_mem);
>   }
>   
>   static void qxl_bo_delete_mem_notify(struct ttm_buffer_object *bo)
>   {
> -	qxl_bo_move_notify(bo, false, NULL);
> +	qxl_bo_move_notify(bo, NULL);
>   }
>   
>   static struct ttm_device_funcs qxl_bo_driver = {
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/vmwgfx: clean up vmw_move_notify

[Alex Deucher]

On Thu, Feb 11, 2021 at 8:17 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Instead of swapping bo->mem just give old and new as parameters.
>
> Also drop unused parameters and code.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h        |  3 ++-
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |  9 +++++---
>  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 26 +++++++---------------
>  3 files changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index e65b00f8336d..21a72fa7b9fd 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> @@ -775,7 +775,8 @@ extern void vmw_resource_unreserve(struct vmw_resource *res,
>                                    struct vmw_buffer_object *new_backup,
>                                    unsigned long new_backup_offset);
>  extern void vmw_query_move_notify(struct ttm_buffer_object *bo,
> -                                 struct ttm_resource *mem);
> +                                 struct ttm_resource *old_mem,
> +                                 struct ttm_resource *new_mem);
>  extern int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob);
>  extern void vmw_resource_evict_all(struct vmw_private *dev_priv);
>  extern void vmw_resource_unbind_list(struct vmw_buffer_object *vbo);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> index c3a724e37104..35f02958ee2c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
> @@ -847,13 +847,15 @@ int vmw_query_readback_all(struct vmw_buffer_object *dx_query_mob)
>   * vmw_query_move_notify - Read back cached query states
>   *
>   * @bo: The TTM buffer object about to move.
> - * @mem: The memory region @bo is moving to.
> + * @old_mem: The memory region @bo is moving from.
> + * @new_mem: The memory region @bo is moving to.
>   *
>   * Called before the query MOB is swapped out to read back cached query
>   * states from the device.
>   */
>  void vmw_query_move_notify(struct ttm_buffer_object *bo,
> -                          struct ttm_resource *mem)
> +                          struct ttm_resource *old_mem,
> +                          struct ttm_resource *new_mem)
>  {
>         struct vmw_buffer_object *dx_query_mob;
>         struct ttm_device *bdev = bo->bdev;
> @@ -871,7 +873,8 @@ void vmw_query_move_notify(struct ttm_buffer_object *bo,
>         }
>
>         /* If BO is being moved from MOB to system memory */
> -       if (mem->mem_type == TTM_PL_SYSTEM && bo->mem.mem_type == VMW_PL_MOB) {
> +       if (new_mem->mem_type == TTM_PL_SYSTEM &&
> +           old_mem->mem_type == VMW_PL_MOB) {
>                 struct vmw_fence_obj *fence;
>
>                 (void) vmw_query_readback_all(dx_query_mob);
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> index 63f10c865061..38801462134e 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> @@ -692,20 +692,19 @@ static int vmw_ttm_io_mem_reserve(struct ttm_device *bdev, struct ttm_resource *
>   *
>   * @bo: The TTM buffer object about to move.
>   * @evict: Unused

Evict is removed now.  Can be dropped from the documentation.

Alex

> - * @mem: The struct ttm_resource indicating to what memory
> + * @old_mem: The old memory where we move from
> + * @new_mem: The struct ttm_resource indicating to what memory
>   *       region the move is taking place.
>   *
>   * Calls move_notify for all subsystems needing it.
>   * (currently only resources).
>   */
>  static void vmw_move_notify(struct ttm_buffer_object *bo,
> -                           bool evict,
> -                           struct ttm_resource *mem)
> +                           struct ttm_resource *old_mem,
> +                           struct ttm_resource *new_mem)
>  {
> -       if (!mem)
> -               return;
> -       vmw_bo_move_notify(bo, mem);
> -       vmw_query_move_notify(bo, mem);
> +       vmw_bo_move_notify(bo, new_mem);
> +       vmw_query_move_notify(bo, old_mem, new_mem);
>  }
>
>
> @@ -736,7 +735,7 @@ static int vmw_move(struct ttm_buffer_object *bo,
>                         return ret;
>         }
>
> -       vmw_move_notify(bo, evict, new_mem);
> +       vmw_move_notify(bo, &bo->mem, new_mem);
>
>         if (old_man->use_tt && new_man->use_tt) {
>                 if (bo->mem.mem_type == TTM_PL_SYSTEM) {
> @@ -758,18 +757,10 @@ static int vmw_move(struct ttm_buffer_object *bo,
>         }
>         return 0;
>  fail:
> -       swap(*new_mem, bo->mem);
> -       vmw_move_notify(bo, false, new_mem);
> -       swap(*new_mem, bo->mem);
> +       vmw_move_notify(bo, new_mem, &bo->mem);
>         return ret;
>  }
>
> -static void
> -vmw_delete_mem_notify(struct ttm_buffer_object *bo)
> -{
> -       vmw_move_notify(bo, false, NULL);
> -}
> -
>  struct ttm_device_funcs vmw_bo_driver = {
>         .ttm_tt_create = &vmw_ttm_tt_create,
>         .ttm_tt_populate = &vmw_ttm_populate,
> @@ -779,7 +770,6 @@ struct ttm_device_funcs vmw_bo_driver = {
>         .evict_flags = vmw_evict_flags,
>         .move = vmw_move,
>         .verify_access = vmw_verify_access,
> -       .delete_mem_notify = vmw_delete_mem_notify,
>         .swap_notify = vmw_swap_notify,
>         .io_mem_reserve = &vmw_ttm_io_mem_reserve,
>  };
> --
> 2.25.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Christian König]

Am 11.02.21 um 15:52 schrieb Thomas Zimmermann:
> Hi
>
> Am 11.02.21 um 14:16 schrieb Christian König:
>> Swapping bo->mem was completely unecessary. Cleanup the function which
>> is just a leftover from a TTM cleanup.
>
> Yes this was introduced in a recent cleanup effort. Can you explain 
> what the code intends to do? It seems as if it tries to "re-unmap the 
> BO" if the move_memcpy fails.
>
> If the move_memcpy fails now, it seems like we can live without 
> reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)

I think so, but I'm not 100% sure either.

The swap() -> notify -> swap() was just how TTM did it and that was 
moved into the drivers.

I'm now just trying to remove all the hard write accesses to bo->mem 
from drivers and stumbled over this here.

Thanks for the comments,
Christian.

>
> Best regards
> Thomas
>
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>> index a0992f0b8afd..0c2233ee6029 100644
>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>> @@ -564,9 +564,7 @@ static void 
>> drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
>>       *pl = gbo->placement;
>>   }
>>   -static void drm_gem_vram_bo_driver_move_notify(struct 
>> drm_gem_vram_object *gbo,
>> -                           bool evict,
>> -                           struct ttm_resource *new_mem)
>> +static void drm_gem_vram_bo_driver_move_notify(struct 
>> drm_gem_vram_object *gbo)
>>   {
>>       struct ttm_buffer_object *bo = &gbo->bo;
>>       struct drm_device *dev = bo->base.dev;
>> @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct 
>> drm_gem_vram_object *gbo,
>>                          struct ttm_operation_ctx *ctx,
>>                          struct ttm_resource *new_mem)
>>   {
>> -    int ret;
>> -
>> -    drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
>> -    ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>> -    if (ret) {
>> -        swap(*new_mem, gbo->bo.mem);
>> -        drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
>> -        swap(*new_mem, gbo->bo.mem);
>> -    }
>> -    return ret;
>> +    drm_gem_vram_bo_driver_move_notify(gbo);
>> +    return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>>   }
>>     /*
>> @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct 
>> ttm_buffer_object *bo)
>>         gbo = drm_gem_vram_of_bo(bo);
>>   -    drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
>> +    drm_gem_vram_bo_driver_move_notify(gbo);
>>   }
>>     static int bo_driver_move(struct ttm_buffer_object *bo,
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3879 bytes --]

Hi

Am 11.02.21 um 16:05 schrieb Christian König:
> 
> 
> Am 11.02.21 um 15:52 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 11.02.21 um 14:16 schrieb Christian König:
>>> Swapping bo->mem was completely unecessary. Cleanup the function which
>>> is just a leftover from a TTM cleanup.
>>
>> Yes this was introduced in a recent cleanup effort. Can you explain 
>> what the code intends to do? It seems as if it tries to "re-unmap the 
>> BO" if the move_memcpy fails.
>>
>> If the move_memcpy fails now, it seems like we can live without 
>> reverting that call to drm_gem_vram_bo_driver_move_notify(). (?)
> 
> I think so, but I'm not 100% sure either.
> 
> The swap() -> notify -> swap() was just how TTM did it and that was 
> moved into the drivers.
> 
> I'm now just trying to remove all the hard write accesses to bo->mem 
> from drivers and stumbled over this here.

We already have a vmap count of zero; so unmapping the BO pages is fine 
at any time. The next caller of vmap will simple instantiate a new mapping.

Let me give this patch a test run tomorrow, but it seems correct.

Best regards
Thomas

> 
> Thanks for the comments,
> Christian.
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
>>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c 
>>> b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> index a0992f0b8afd..0c2233ee6029 100644
>>> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
>>> @@ -564,9 +564,7 @@ static void 
>>> drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
>>>       *pl = gbo->placement;
>>>   }
>>>   -static void drm_gem_vram_bo_driver_move_notify(struct 
>>> drm_gem_vram_object *gbo,
>>> -                           bool evict,
>>> -                           struct ttm_resource *new_mem)
>>> +static void drm_gem_vram_bo_driver_move_notify(struct 
>>> drm_gem_vram_object *gbo)
>>>   {
>>>       struct ttm_buffer_object *bo = &gbo->bo;
>>>       struct drm_device *dev = bo->base.dev;
>>> @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct 
>>> drm_gem_vram_object *gbo,
>>>                          struct ttm_operation_ctx *ctx,
>>>                          struct ttm_resource *new_mem)
>>>   {
>>> -    int ret;
>>> -
>>> -    drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
>>> -    ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>>> -    if (ret) {
>>> -        swap(*new_mem, gbo->bo.mem);
>>> -        drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
>>> -        swap(*new_mem, gbo->bo.mem);
>>> -    }
>>> -    return ret;
>>> +    drm_gem_vram_bo_driver_move_notify(gbo);
>>> +    return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>>>   }
>>>     /*
>>> @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct 
>>> ttm_buffer_object *bo)
>>>         gbo = drm_gem_vram_of_bo(bo);
>>>   -    drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
>>> +    drm_gem_vram_bo_driver_move_notify(gbo);
>>>   }
>>>     static int bo_driver_move(struct ttm_buffer_object *bo,
>>>
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/vram-helper: cleanup drm_gem_vram_bo_driver_move_notify

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 2366 bytes --]

Hi

Am 11.02.21 um 14:16 schrieb Christian König:
> Swapping bo->mem was completely unecessary. Cleanup the function which
> is just a leftover from a TTM cleanup.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Appears to work.

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_gem_vram_helper.c | 18 ++++--------------
>   1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index a0992f0b8afd..0c2233ee6029 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -564,9 +564,7 @@ static void drm_gem_vram_bo_driver_evict_flags(struct drm_gem_vram_object *gbo,
>   	*pl = gbo->placement;
>   }
>   
> -static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo,
> -					       bool evict,
> -					       struct ttm_resource *new_mem)
> +static void drm_gem_vram_bo_driver_move_notify(struct drm_gem_vram_object *gbo)
>   {
>   	struct ttm_buffer_object *bo = &gbo->bo;
>   	struct drm_device *dev = bo->base.dev;
> @@ -582,16 +580,8 @@ static int drm_gem_vram_bo_driver_move(struct drm_gem_vram_object *gbo,
>   				       struct ttm_operation_ctx *ctx,
>   				       struct ttm_resource *new_mem)
>   {
> -	int ret;
> -
> -	drm_gem_vram_bo_driver_move_notify(gbo, evict, new_mem);
> -	ret = ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
> -	if (ret) {
> -		swap(*new_mem, gbo->bo.mem);
> -		drm_gem_vram_bo_driver_move_notify(gbo, false, new_mem);
> -		swap(*new_mem, gbo->bo.mem);
> -	}
> -	return ret;
> +	drm_gem_vram_bo_driver_move_notify(gbo);
> +	return ttm_bo_move_memcpy(&gbo->bo, ctx, new_mem);
>   }
>   
>   /*
> @@ -947,7 +937,7 @@ static void bo_driver_delete_mem_notify(struct ttm_buffer_object *bo)
>   
>   	gbo = drm_gem_vram_of_bo(bo);
>   
> -	drm_gem_vram_bo_driver_move_notify(gbo, false, NULL);
> +	drm_gem_vram_bo_driver_move_notify(gbo);
>   }
>   
>   static int bo_driver_move(struct ttm_buffer_object *bo,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel