[PATCH 2/5] drm/gem: Add a mountpoint parameter to drm_gem_object_init()

[Christian König]

Am 12.03.24 um 14:09 schrieb Tvrtko Ursulin:
>
> On 12/03/2024 10:37, Christian König wrote:
>> Am 12.03.24 um 11:31 schrieb Tvrtko Ursulin:
>>>
>>> On 12/03/2024 10:23, Christian König wrote:
>>>> Am 12.03.24 um 10:30 schrieb Tvrtko Ursulin:
>>>>>
>>>>> On 12/03/2024 08:59, Christian König wrote:
>>>>>> Am 12.03.24 um 09:51 schrieb Tvrtko Ursulin:
>>>>>>>
>>>>>>> Hi Maira,
>>>>>>>
>>>>>>> On 11/03/2024 10:05, Maíra Canal wrote:
>>>>>>>> For some applications, such as using huge pages, we might want 
>>>>>>>> to have a
>>>>>>>> different mountpoint, for which we pass in mount flags that 
>>>>>>>> better match
>>>>>>>> our usecase.
>>>>>>>>
>>>>>>>> Therefore, add a new parameter to drm_gem_object_init() that 
>>>>>>>> allow us to
>>>>>>>> define the tmpfs mountpoint where the GEM object will be 
>>>>>>>> created. If
>>>>>>>> this parameter is NULL, then we fallback to shmem_file_setup().
>>>>>>>
>>>>>>> One strategy for reducing churn, and so the number of drivers 
>>>>>>> this patch touches, could be to add a lower level 
>>>>>>> drm_gem_object_init() (which takes vfsmount, call it 
>>>>>>> __drm_gem_object_init(), or drm__gem_object_init_mnt(), and make 
>>>>>>> drm_gem_object_init() call that one with a NULL argument.
>>>>>>
>>>>>> I would even go a step further into the other direction. The 
>>>>>> shmem backed GEM object is just some special handling as far as I 
>>>>>> can see.
>>>>>>
>>>>>> So I would rather suggest to rename all drm_gem_* function which 
>>>>>> only deal with the shmem backed GEM object into drm_gem_shmem_*.
>>>>>
>>>>> That makes sense although it would be very churny. I at least 
>>>>> would be on the fence regarding the cost vs benefit.
>>>>
>>>> Yeah, it should clearly not be part of this patch here.
>>>>
>>>>>
>>>>>> Also the explanation why a different mount point helps with 
>>>>>> something isn't very satisfying.
>>>>>
>>>>> Not satisfying as you think it is not detailed enough to say 
>>>>> driver wants to use huge pages for performance? Or not satisying 
>>>>> as you question why huge pages would help?
>>>>
>>>> That huge pages are beneficial is clear to me, but I'm missing the 
>>>> connection why a different mount point helps with using huge pages.
>>>
>>> Ah right, same as in i915, one needs to mount a tmpfs instance 
>>> passing huge=within_size or huge=always option. Default is 'never', 
>>> see man 5 tmpfs.
>>
>> Thanks for the explanation, I wasn't aware of that.
>>
>> Mhm, shouldn't we always use huge pages? Is there a reason for a DRM 
>> device to not use huge pages with the shmem backend?
>
> AFAIU, according to b901bb89324a ("drm/i915/gemfs: enable THP"), back 
> then the understanding was within_size may overallocate, meaning there 
> would be some space wastage, until the memory pressure makes the thp 
> code split the trailing huge page. I haven't checked if that still 
> applies.
>
> Other than that I don't know if some drivers/platforms could have 
> problems if they have some limitations or hardcoded assumptions when 
> they iterate the sg list.

Yeah, that was the whole point behind my question. As far as I can see 
this isn't driver specific, but platform specific.

I might be wrong here, but I think we should then probably not have that 
handling in each individual driver, but rather centralized in the DRM code.

Regards,
Christian.


>
> Te Cc is plenty large so perhaps someone else will have additional 
> information. :)
>
> Regards,
>
> Tvrtko
>
>>
>> I mean it would make this patch here even smaller.
>>
>> Regards,
>> Christian.
>>
>>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>