Re: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
Am 19.05.22 um 15:36 schrieb Ruhl, Michael J: -Original Message- From: dri-devel On Behalf Of Christian König Sent: Thursday, May 19, 2022 5:55 AM To: intel-...@lists.freedesktop.org Cc: matthew.william.a...@gmail.com; Christian König ; dri-devel@lists.freedesktop.org Subject: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX It's the only driver using this. Signed-off-by: Christian König --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index 85a66014c2b6..c4f376d5e1d0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -462,6 +462,9 @@ int vmw_bo_create(struct vmw_private *vmw, return -ENOMEM; } + if (!bo_free) + bo_free = vmw_bo_default_destroy; + vmw_bo_init has a WARN_ON if this is NULL. Also, all of the callers use vmw_bo_bo_free() or vmw_gem_destroy(). Both of those unmap, release and then free the object. It doesn't look like vmw_bo_default_destroy does this work. Is this the right "default" path? Or should the WARN_ON be used to check for this? This patch here was just a rebase fallout I've overlooked. Zak already reviewed it and I pushed a modified version of it upstream (that's where the WARN_ON comes from). Thanks, Christian. M ret = vmw_bo_init(vmw, *p_bo, size, placement, interruptible, pin, bo_free); -- 2.25.1
RE: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
>-Original Message- >From: dri-devel On Behalf Of >Christian König >Sent: Thursday, May 19, 2022 5:55 AM >To: intel-...@lists.freedesktop.org >Cc: matthew.william.a...@gmail.com; Christian König >; dri-devel@lists.freedesktop.org >Subject: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX > >It's the only driver using this. > >Signed-off-by: Christian König >--- > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 3 +++ > 1 file changed, 3 insertions(+) > >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c >b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c >index 85a66014c2b6..c4f376d5e1d0 100644 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c >@@ -462,6 +462,9 @@ int vmw_bo_create(struct vmw_private *vmw, > return -ENOMEM; > } > >+ if (!bo_free) >+ bo_free = vmw_bo_default_destroy; >+ vmw_bo_init has a WARN_ON if this is NULL. Also, all of the callers use vmw_bo_bo_free() or vmw_gem_destroy(). Both of those unmap, release and then free the object. It doesn't look like vmw_bo_default_destroy does this work. Is this the right "default" path? Or should the WARN_ON be used to check for this? M > ret = vmw_bo_init(vmw, *p_bo, size, > placement, interruptible, pin, > bo_free); >-- >2.25.1
[PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
It's the only driver using this. Signed-off-by: Christian König --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index 85a66014c2b6..c4f376d5e1d0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -462,6 +462,9 @@ int vmw_bo_create(struct vmw_private *vmw, return -ENOMEM; } + if (!bo_free) + bo_free = vmw_bo_default_destroy; + ret = vmw_bo_init(vmw, *p_bo, size, placement, interruptible, pin, bo_free); -- 2.25.1
[PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
It's the only driver using this. Signed-off-by: Christian König --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c index 85a66014c2b6..c4f376d5e1d0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c @@ -462,6 +462,9 @@ int vmw_bo_create(struct vmw_private *vmw, return -ENOMEM; } + if (!bo_free) + bo_free = vmw_bo_default_destroy; + ret = vmw_bo_init(vmw, *p_bo, size, placement, interruptible, pin, bo_free); -- 2.25.1
Re: [PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
On Tue, 2022-03-29 at 13:02 +0200, Christian König wrote:
> ⚠ External Email
>
> It's the only driver using this.
>
> Signed-off-by: Christian König
Looks good. A small suggestion underneath.
Reviewed-by: Zack Rusin
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 9 +
> drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 11 ++-
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> b/drivers/gpu/drm/ttm/ttm_bo.c
> index e5fd0f2c0299..7598d59423bf 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -44,12 +44,6 @@
>
> #include "ttm_module.h"
>
> -/* default destructor */
> -static void ttm_bo_default_destroy(struct ttm_buffer_object *bo)
> -{
> - kfree(bo);
> -}
> -
> static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
> struct ttm_placement
> *placement)
> {
> @@ -938,8 +932,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
> bool locked;
> int ret;
>
> - bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
> -
> + bo->destroy = destroy;
> kref_init(>kref);
> INIT_LIST_HEAD(>ddestroy);
> bo->bdev = bdev;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> index 31aecc46624b..60dcc6a75248 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
> @@ -378,6 +378,12 @@ void vmw_bo_bo_free(struct ttm_buffer_object
> *bo)
> kfree(vmw_bo);
> }
>
> +/* default destructor */
> +static void vmw_bo_default_destroy(struct ttm_buffer_object *bo)
> +{
> + kfree(bo);
> +}
> +
> /**
> * vmw_bo_create_kernel - Create a pinned BO for internal kernel
> use.
> *
> @@ -410,7 +416,7 @@ int vmw_bo_create_kernel(struct vmw_private
> *dev_priv, unsigned long size,
>
> ret = ttm_bo_init_reserved(_priv->bdev, bo, size,
> ttm_bo_type_kernel, placement, 0,
> - , NULL, NULL, NULL);
> + , NULL, NULL,
> vmw_bo_default_destroy);
> if (unlikely(ret))
> goto error_free;
>
> @@ -439,6 +445,9 @@ int vmw_bo_create(struct vmw_private *vmw,
> return -ENOMEM;
> }
>
> + if (!bo_free)
> + bo_free = vmw_bo_default_destroy;
> +
If you could change this to just BUG_ON(!bo_free) that'd be great.
bo_free == NULL should never happen here.
z
[PATCH 04/11] drm/ttm: move default BO destructor into VMWGFX
It's the only driver using this.
Signed-off-by: Christian König
---
drivers/gpu/drm/ttm/ttm_bo.c | 9 +
drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 11 ++-
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e5fd0f2c0299..7598d59423bf 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -44,12 +44,6 @@
#include "ttm_module.h"
-/* default destructor */
-static void ttm_bo_default_destroy(struct ttm_buffer_object *bo)
-{
- kfree(bo);
-}
-
static void ttm_bo_mem_space_debug(struct ttm_buffer_object *bo,
struct ttm_placement *placement)
{
@@ -938,8 +932,7 @@ int ttm_bo_init_reserved(struct ttm_device *bdev,
bool locked;
int ret;
- bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
-
+ bo->destroy = destroy;
kref_init(>kref);
INIT_LIST_HEAD(>ddestroy);
bo->bdev = bdev;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
index 31aecc46624b..60dcc6a75248 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
@@ -378,6 +378,12 @@ void vmw_bo_bo_free(struct ttm_buffer_object *bo)
kfree(vmw_bo);
}
+/* default destructor */
+static void vmw_bo_default_destroy(struct ttm_buffer_object *bo)
+{
+ kfree(bo);
+}
+
/**
* vmw_bo_create_kernel - Create a pinned BO for internal kernel use.
*
@@ -410,7 +416,7 @@ int vmw_bo_create_kernel(struct vmw_private *dev_priv,
unsigned long size,
ret = ttm_bo_init_reserved(_priv->bdev, bo, size,
ttm_bo_type_kernel, placement, 0,
- , NULL, NULL, NULL);
+ , NULL, NULL, vmw_bo_default_destroy);
if (unlikely(ret))
goto error_free;
@@ -439,6 +445,9 @@ int vmw_bo_create(struct vmw_private *vmw,
return -ENOMEM;
}
+ if (!bo_free)
+ bo_free = vmw_bo_default_destroy;
+
ret = vmw_bo_init(vmw, *p_bo, size,
placement, interruptible, pin,
bo_free);
--
2.25.1
