Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()

[suijingfeng]

Hi,

On 2023/7/18 16:12, Lucas Stach wrote:
> Hi Jingfeng,
>
> Am Dienstag, dem 18.07.2023 um 02:34 +0800 schrieb suijingfeng:
> > Hi,  Lucas
> >
> >
> > Thanks for you guidance!
> >
> >
> > On 2023/7/17 17:51, Lucas Stach wrote:
> > > Hi Jingfeng,
> > >
> > > Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> > > > From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > > >
> > > > Because it is not used by the etnaviv_gem_new_impl() function,
> > > > no functional change.
> > > I think it would make sense to move into the opposite direction: in
> > > both callsites of etnaviv_gem_new_impl we immediately call
> > > drm_gem_object_init with the size.
> > Really?
> >
> > But there are multiple call path to the etnaviv_gem_new_impl() function.
> >
> >
> > Code path 1 (PRIME):
> >
> > > - etnaviv_gem_prime_import_sg_table()
> > > --  etnaviv_gem_new_private()
> > > --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
> > > --- drm_gem_private_object_init(dev, obj, size)
> >
> > Code path 2 (USERPTR):
> >
> > > - etnaviv_gem_new_userptr()
> > > --  etnaviv_gem_new_private()
> > > --- etnaviv_gem_new_impl(dev, size, flags, ops, &obj)
> > > --- drm_gem_private_object_init(dev, obj, size)
> >
> > Code path 3 (construct a GEM buffer object for the user-space):
> >
> > > - etnaviv_ioctl_gem_new()
> > > -- etnaviv_gem_new_handle()
> > > --- etnaviv_gem_new_impl(dev, size, flags, &etnaviv_gem_shmem_ops, &obj);
> > > ---  drm_gem_object_init(dev, obj, size);
> >
> > If I understand this correctly:
> >
> >
> > Code path 1 is for cross device (and cross driver) buffer-sharing,
> >
> > Code path 2 is going to share the buffer the userspace,
> >
> >
> > *Only* the code path 3 is to construct a GEM buffer object for the
> > user-space the userspace,
> >
> > that is say, *only* the code path 3 need to do the backing memory
> > allocation work for the userspace.
> >
> > thus it need to call drm_gem_object_init() function, which really the
> > shmem do the backing memory
> >
> > allocation.
> >
> >
> > The code path 1 and the code path 2 do not need the kernel space
> > allocate the backing memory.
> >
> > Because they are going to share the buffer already allocated by others.
> >
> > thus, code path 2 and code path 3 should call drm_gem_private_object_init(),
> >
> > *not* the drm_gem_object_init() function.
> >
> >
> > When import buffer from the a specific KMS driver,
> >
> > then etnaviv_gem_prime_import_sg_table() will be called.
> >
> >
> > I guess you means that drm_gem_private_object_init() (not the
> > drm_gem_object_init() function)here ?
> >
> >
> > > A better cleanup would be to make
> > > use of the size parameter and move this object init call into
> > > etnaviv_gem_new_impl.
> > If I following you guidance, how do I differentiate the cases
> >
> > when to call drm_gem_private_object_init() not drm_gem_object_init() ?
> >
> > and when call drm_gem_object_init() not drm_gem_private_object_init()?
> >
> >
> > I don't think you are right here.
> Yes, clearly I was not taking into account the differences between
> drm_gem_private_object_init and drm_gem_object_init properly. Please
> disregard my comment, this patch is good as-is.

I have study your patch in the past frequently.

As you could solve very complex(and difficulty) bugs.

So I still believe that you know everything about etnaviv.

I'm just wondering that you are designing the traps. But I'm not sure.

Okay, still acceptable.

Because communicate will you is interesting.

Thank you.

> Regards,
> Lucas
>
> > > Regards,
> > > Lucas
> > >
> > > > Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> > > > ---
> > > >    drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++----
> > > >    1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > index b5f73502e3dd..be2f459c66b5 100644
> > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > > @@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
> > > >    	.vm_ops = &vm_ops,
> > > >    };
> > > >    
> > > > -static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
> > > > +static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
> > > >    	const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
> > > >    {
> > > >    	struct etnaviv_gem_object *etnaviv_obj;
> > > > @@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct drm_file *file,
> > > >    
> > > >    	size = PAGE_ALIGN(size);
> > > >    
> > > > -	ret = etnaviv_gem_new_impl(dev, size, flags,
> > > > -				   &etnaviv_gem_shmem_ops, &obj);
> > > > +	ret = etnaviv_gem_new_impl(dev, flags, &etnaviv_gem_shmem_ops, &obj);
> > > >    	if (ret)
> > > >    		goto fail;
> > > >    
> > > > @@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
> > > >    	struct drm_gem_object *obj;
> > > >    	int ret;
> > > >    
> > > > -	ret = etnaviv_gem_new_impl(dev, size, flags, ops, &obj);
> > > > +	ret = etnaviv_gem_new_impl(dev, flags, ops, &obj);
> > > >    	if (ret)
> > > >    		return ret;
> > > >