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

[Sui Jingfeng <sui.jingfeng@xxxxxxxxx>]

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