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

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

 



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;




[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