Re: 回复: [PATCH Review 1/1] drm/ttm: fix debugfs node create failed

[Christian König]

Am 19.10.21 um 10:02 schrieb Yang, Stanley:
> [AMD Official Use Only]
>
>
>> -----邮件原件-----
>> 发件人: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> 代表 Das,
>> Nirmoy
>> 发送时间: Thursday, October 14, 2021 2:11 AM
>> 收件人: Christian König <ckoenig.leichtzumerken at gmail.com>; amd-
>> gfx at lists.freedesktop.org
>> 主题: Re: [PATCH Review 1/1] drm/ttm: fix debugfs node create failed
>>
>>
>> On 10/13/2021 2:29 PM, Christian König wrote:
>>> Am 12.10.21 um 15:12 schrieb Das, Nirmoy:
>>>> On 10/12/2021 1:58 PM, Stanley.Yang wrote:
>>>>> Test scenario:
>>>>>       modprobe amdgpu -> rmmod amdgpu -> modprobe amdgpu Error log:
>>>>>       [   54.396807] debugfs: File 'page_pool' in directory 'amdttm'
>>>>> already present!
>>>>>       [   54.396833] debugfs: File 'page_pool_shrink' in directory
>>>>> 'amdttm' already present!
>>>>>       [   54.396848] debugfs: File 'buffer_objects' in directory
>>>>> 'amdttm' already present!
>>>>
>>>> We should instead add a check if those debugfs files already
>>>> exist/created in ttm debugfs dir using debugfs_lookup() before creating.
>>> No, IIRC the Intel guys had fixed that already by adding/removing the
>>> debugfs file on module load/unload.
>>
>> Adding/removing on ttm module load/unload is nicer.
> The point is that page_pool, page_pool_shrink and buffer_objects are created by amdgpu driver,

Yeah, but the debugfs files are not created by the driver. Those are 
global to TTM and can trivially be created during module load/unload.

Christian.

>   I think it's better to remove them by amdgpu module due to amdgpu module create them,
> otherwise, there will be a scene create them failed only reload amdgpu module.
>
> Stanley
>>
>> Nirmoy
>>
>>>
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>>
>>>> Nirmoy
>>>>
>>>>
>>>>
>>>>> Reason:
>>>>>       page_pool, page_pool_shrink and buffer_objects can be removed
>>>>> when
>>>>>       rmmod amdttm, in the above test scenario only rmmod amdgpu, so
>>>>> those
>>>>>       debugfs node will not be removed, this caused file create failed.
>>>>> Soultion:
>>>>>       create ttm_page directory under ttm_root directory when insmod
>>>>> amdgpu,
>>>>>       page_pool, page_pool_shrink and buffer_objects are stored in
>>>>> ttm_page directiry,
>>>>>       remove ttm_page directory when do rmmod amdgpu, this can fix
>>>>> above issue.
>>>>>
>>>>> Signed-off-by: Stanley.Yang <Stanley.Yang at amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/ttm/ttm_device.c | 12 +++++++++++-
>>>>>    drivers/gpu/drm/ttm/ttm_module.c |  1 +
>>>>>    drivers/gpu/drm/ttm/ttm_module.h |  1 +
>>>>>    drivers/gpu/drm/ttm/ttm_pool.c   |  4 ++--
>>>>>    4 files changed, 15 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_device.c
>>>>> b/drivers/gpu/drm/ttm/ttm_device.c
>>>>> index 1de23edbc182..ad170328f0c8 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_device.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_device.c
>>>>> @@ -55,6 +55,10 @@ static void ttm_global_release(void)
>>>>>          ttm_pool_mgr_fini();
>>>>>    +#ifdef CONFIG_DEBUG_FS
>>>>> +    debugfs_remove(ttm_debugfs_page); #endif
>>>>> +
>>>>>        __free_page(glob->dummy_read_page);
>>>>>        memset(glob, 0, sizeof(*glob));
>>>>>    out:
>>>>> @@ -85,6 +89,10 @@ static int ttm_global_init(void)
>>>>>            >> PAGE_SHIFT;
>>>>>        num_dma32 = min(num_dma32, 2UL << (30 - PAGE_SHIFT));
>>>>>    +#ifdef CONFIG_DEBUG_FS
>>>>> +    ttm_debugfs_page = debugfs_create_dir("ttm_page",
>>>>> ttm_debugfs_root);
>>>>> +#endif
>>>>> +
>>>>>        ttm_pool_mgr_init(num_pages);
>>>>>        ttm_tt_mgr_init(num_pages, num_dma32);
>>>>>    @@ -98,8 +106,10 @@ static int ttm_global_init(void)
>>>>>        INIT_LIST_HEAD(&glob->device_list);
>>>>>        atomic_set(&glob->bo_count, 0);
>>>>>    -    debugfs_create_atomic_t("buffer_objects", 0444,
>>>>> ttm_debugfs_root,
>>>>> +#ifdef CONFIG_DEBUG_FS
>>>>> +    debugfs_create_atomic_t("buffer_objects", 0444,
>>>>> +ttm_debugfs_page,
>>>>>                    &glob->bo_count);
>>>>> +#endif
>>>>>    out:
>>>>>        mutex_unlock(&ttm_global_mutex);
>>>>>        return ret;
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_module.c
>>>>> b/drivers/gpu/drm/ttm/ttm_module.c
>>>>> index 88970a6b8e32..66595e6e7087 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_module.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_module.c
>>>>> @@ -38,6 +38,7 @@
>>>>>    #include "ttm_module.h"
>>>>>      struct dentry *ttm_debugfs_root;
>>>>> +struct dentry *ttm_debugfs_page;
>>>>>      static int __init ttm_init(void)
>>>>>    {
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_module.h
>>>>> b/drivers/gpu/drm/ttm/ttm_module.h
>>>>> index d7cac5d4b835..6007dc66f44e 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_module.h
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_module.h
>>>>> @@ -36,5 +36,6 @@
>>>>>    struct dentry;
>>>>>      extern struct dentry *ttm_debugfs_root;
>>>>> +extern struct dentry *ttm_debugfs_page;
>>>>>      #endif /* _TTM_MODULE_H_ */
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> b/drivers/gpu/drm/ttm/ttm_pool.c index 8be7fd7161fd..ecb33daad7b5
>>>>> 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>>>> @@ -709,9 +709,9 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>>>>>        }
>>>>>      #ifdef CONFIG_DEBUG_FS
>>>>> -    debugfs_create_file("page_pool", 0444, ttm_debugfs_root, NULL,
>>>>> +    debugfs_create_file("page_pool", 0444, ttm_debugfs_page, NULL,
>>>>>                    &ttm_pool_debugfs_globals_fops);
>>>>> -    debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_root,
>>>>> NULL,
>>>>> +    debugfs_create_file("page_pool_shrink", 0400, ttm_debugfs_page,
>>>>> NULL,
>>>>>                    &ttm_pool_debugfs_shrink_fops);
>>>>>    #endif