Re: [PATCH v15 16/19] drm/etnaviv: Call etnaviv_gem_obj_add() in ernaviv_gem_new_private()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 2024/10/1 22:39, Lucas Stach wrote:
Am Sonntag, dem 08.09.2024 um 17:43 +0800 schrieb Sui Jingfeng:
The etnaviv_gem_obj_add() a common operation, the 'etnaviv_drm_private::
gem_list' is being used to record(track) all of the etnaviv GEM buffer
object created in this driver.

Once a etnaviv GEM buffer object has been allocated successfully, we
should add it into the global etnaviv_drm_private::gem_list'. Because
we need to free it and remove it free the list if the invoke of the
subsequent functions fail.

The only way that destroy etnaviv GEM buffer object is the implementation
of etnaviv_gem_free_object() function. The etnaviv_gem_free_object() first
remove the etnaviv GEM object from the list, then destroy its mapping and
finally free its memory footprint. Therefore, in order to corresponding
this, we add the freshly created etnaviv GEM buffer object immediately
after it was successfully created.

A benifit is that we only need to call etnaviv_gem_obj_add() once now,
since the ernaviv_gem_new_private() has been unified. Make the
etnaviv_gem_obj_add() static is a next nature thing.

Seeing this patch, I would really ask you to drop patch 11 from this
series and go the other way around: remove etnaviv_gem_obj_add() here
altogether and simply inline the few lines of code into
etnaviv_gem_new_private().


The 'etnaviv_drm_private::gem_list' is being used to manage the buffer
object itself, not the backing memory of a specific buffer object.

Therefore, once a etnaviv GEM buffer object is successfully created,
we want to add it into the global etnaviv_drm_private::gem_list'.
To prevent the cases which removing an entry that is not in the list.

Both etnaviv_gem_obj_add() and etnaviv_gem_obj_remove() touches driver
private but shared data members. Which has nothing to do with the GEM
core functionality itself.

The unction pair is for tracking, recording, listing and showing
purpose. Which is clearly separated functionality. Split them from
the BO construction is a untangled fashion and is a good thing.


Regards,
Lucas

Signed-off-by: Sui Jingfeng <sui.jingfeng@xxxxxxxxx>
---
  drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 8 +++-----
  drivers/gpu/drm/etnaviv/etnaviv_gem.h       | 1 -
  drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 2 --
  3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 27e4a93c981c..ee799c02d0aa 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -584,7 +584,7 @@ void etnaviv_gem_free_object(struct drm_gem_object *obj)
  	kfree(etnaviv_obj);
  }
-void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
+static void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj)
  {
  	struct etnaviv_drm_private *priv = to_etnaviv_priv(dev);
  	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
@@ -719,8 +719,6 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
  	 */
  	mapping_set_gfp_mask(obj->filp->f_mapping, priv->shm_gfp_mask);
- etnaviv_gem_obj_add(dev, obj);
-
  	ret = drm_gem_handle_create(file, obj, handle);
/* drop reference from allocate - handle holds it now */
@@ -751,6 +749,8 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
  		drm_gem_private_object_init(dev, obj, size);
  	}
+ etnaviv_gem_obj_add(dev, obj);
+
  	*res = to_etnaviv_bo(obj);
return 0;
@@ -849,8 +849,6 @@ int etnaviv_gem_new_userptr(struct drm_device *dev, struct drm_file *file,
  	etnaviv_obj->userptr.mm = current->mm;
  	etnaviv_obj->userptr.ro = !(flags & ETNA_USERPTR_WRITE);
- etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
-
  	ret = drm_gem_handle_create(file, &etnaviv_obj->base, handle);
/* drop reference from allocate - handle holds it now */
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
index b174a9e4cc48..8d8fc5b3a541 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
@@ -121,7 +121,6 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
  			    bool shmem, const struct etnaviv_gem_ops *ops,
  			    struct etnaviv_gem_object **res);
-void etnaviv_gem_obj_add(struct drm_device *dev, struct drm_gem_object *obj);
  struct page **etnaviv_gem_get_pages(struct etnaviv_gem_object *obj);
  void etnaviv_gem_put_pages(struct etnaviv_gem_object *obj);
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
index 64a858a0b0cf..04ee31461b8c 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
@@ -127,8 +127,6 @@ struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
  	if (ret)
  		goto fail;
- etnaviv_gem_obj_add(dev, &etnaviv_obj->base);
-
  	return &etnaviv_obj->base;
fail:

--
Best regards,
Sui




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux