From: Sui Jingfeng <[email protected]>
Because it is not used by the etnaviv_gem_new_impl() function,
no functional change.
Signed-off-by: Sui Jingfeng <[email protected]>
---
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;
--
2.25.1
Hi Jingfeng,
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <[email protected]>
>
> 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. A better cleanup would be to make
use of the size parameter and move this object init call into
etnaviv_gem_new_impl.
Regards,
Lucas
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> 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;
>
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 <[email protected]>
>>
>> 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.
>
> Regards,
> Lucas
>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> 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;
>>
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 <[email protected]>
> > >
> > > 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.
Regards,
Lucas
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > ---
> > > 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;
> > >
>
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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> 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;
>>>>
On 2023/7/19 00:16, suijingfeng wrote:
> Because communicate will you
'will' -> 'with'
Hi Jingfeng,
Am Mittwoch, dem 19.07.2023 um 00:16 +0800 schrieb suijingfeng:
> Hi,
> > >
[...]
> > > 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.
>
While flattering, even I myself am not thinking I know everything about
etnaviv. The different HW generations and the complex subsystem the
driver is living in doesn't make it easy for anyone to keep in mind
everything.
> I'm just wondering that you are designing the traps. But I'm not sure.
>
Certainly not. I'm just human and do make mistakes as everyone. During
the quick scrolling though the code when reviewing this patch my mind
clearly just dropped the _private_ part of one of the function names. I
appreciate being told when I am wrong and I do believe that the
exchange during the review is helpful for everyone to get on the same
page.
Regards,
Lucas
> Okay, still acceptable.
>
> Because communicate will you is interesting.
>
> Thank you.
>
> > Regards,
> > Lucas
> >
> > > > Regards,
> > > > Lucas
> > > >
> > > > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > > > ---
> > > > > 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;
> > > > >
>