[PATCH 1/5] drm/ttm: Add common debugfs code for resource managers

[Christian König]

Am 08.04.22 um 09:56 schrieb Daniel Vetter:
> On Thu, Apr 07, 2022 at 02:15:52PM +0000, Zack Rusin wrote:
>> On Thu, 2022-04-07 at 08:01 +0200, Christian König wrote:
>>> Am 07.04.22 um 04:56 schrieb Zack Rusin:
>>>> From: Zack Rusin <zackr at vmware.com>
>>>>
>>>> Drivers duplicate the code required to add debugfs entries for
>>>> various
>>>> ttm resource managers. To fix it add common TTM resource manager
>>>> code that each driver can reuse.
>>>>
>>>> Because TTM resource managers can be initialized and set a lot
>>>> later
>>>> than TTM device initialization a seperate init function is
>>>> required.
>>>> Specific resource managers can overwrite
>>>> ttm_resource_manager_func::debug to get more information from those
>>>> debugfs entries.
>>>>
>>>> Signed-off-by: Zack Rusin <zackr at vmware.com>
>>>> Cc: Christian Koenig <christian.koenig at amd.com>
>>>> Cc: Huang Rui <ray.huang at amd.com>
>>>> Cc: David Airlie <airlied at linux.ie>
>>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>> Ah, yes that was on my TODO list for quite a while as well.
>>>
>>>> ---
>>>>    drivers/gpu/drm/ttm/ttm_resource.c | 65
>>>> ++++++++++++++++++++++++++++++
>>>>    include/drm/ttm/ttm_resource.h     |  4 ++
>>>>    2 files changed, 69 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
>>>> b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> index 492ba3157e75..6392ad3e9a88 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_resource.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
>>>> @@ -29,6 +29,8 @@
>>>>    #include <drm/ttm/ttm_resource.h>
>>>>    #include <drm/ttm/ttm_bo_driver.h>
>>>>
>>>> +#include "ttm_module.h"
>>>> +
>>>>    /**
>>>>     * ttm_lru_bulk_move_init - initialize a bulk move structure
>>>>     * @bulk: the structure to init
>>>> @@ -644,3 +646,66 @@ ttm_kmap_iter_linear_io_fini(struct
>>>> ttm_kmap_iter_linear_io *iter_io,
>>>>
>>>>        ttm_mem_io_free(bdev, mem);
>>>>    }
>>>> +
>>>> +#if defined(CONFIG_DEBUG_FS)
>>>> +
>>>> +#define TTM_RES_MAN_SHOW(i) \
>>>> +     static int ttm_resource_manager##i##_show(struct seq_file *m,
>>>> void *unused) \
>>>> +     { \
>>>> +             struct ttm_device *bdev = (struct ttm_device *)m-
>>>>> private; \
>>>> +             struct ttm_resource_manager *man =
>>>> ttm_manager_type(bdev, i); \
>>>> +             struct drm_printer p = drm_seq_file_printer(m); \
>>>> +             ttm_resource_manager_debug(man, &p); \
>>>> +             return 0; \
>>>> +     }\
>>>> +     DEFINE_SHOW_ATTRIBUTE(ttm_resource_manager##i)
>>>> +
>>>> +TTM_RES_MAN_SHOW(0);
>>>> +TTM_RES_MAN_SHOW(1);
>>>> +TTM_RES_MAN_SHOW(2);
>>>> +TTM_RES_MAN_SHOW(3);
>>>> +TTM_RES_MAN_SHOW(4);
>>>> +TTM_RES_MAN_SHOW(5);
>>>> +TTM_RES_MAN_SHOW(6);
>>> Uff, please not a static array.
>>>
>>>> +
>>>> +#endif
>>>> +
>>>> +/**
>>>> + * ttm_resource_manager_debugfs_init - Setup debugfs entries for
>>>> specified
>>>> + * resource managers.
>>>> + * @bdev: The TTM device
>>>> + * @file_names: A mapping between TTM_TT placements and the
>>>> debugfs file
>>>> + * names
>>>> + * @num_file_names: The array size of @file_names.
>>>> + *
>>>> + * This function setups up debugfs files that can be used to look
>>>> + * at debug statistics of the specified ttm_resource_managers.
>>>> + * @file_names array is used to figure out which ttm placements
>>>> + * will get debugfs files created for them.
>>>> + */
>>>> +void
>>>> +ttm_resource_manager_debugfs_init(struct ttm_device *bdev,
>>>> +                               const char * const file_names[],
>>>> +                               uint32_t num_file_names)
>>>> +{
>>>> +#if defined(CONFIG_DEBUG_FS)
>>>> +     uint32_t i;
>>>> +     const struct file_operations *fops[] = {
>>>> +             &ttm_resource_manager0_fops,
>>>> +             &ttm_resource_manager1_fops,
>>>> +             &ttm_resource_manager2_fops,
>>>> +             &ttm_resource_manager3_fops,
>>>> +             &ttm_resource_manager4_fops,
>>>> +             &ttm_resource_manager5_fops,
>>>> +             &ttm_resource_manager6_fops,
>>>> +     };
>>>> +
>>>> +     WARN_ON(num_file_names > ARRAY_SIZE(fops));
>>>> +
>>>> +     for (i = 0; i < num_file_names; ++i)
>>>> +             if (file_names[i] && fops[i])
>>>> +                     debugfs_create_file(file_names[i], 0444,
>>>> +                                         ttm_debugfs_root, bdev,
>>>> fops[i]);
>>> You can give the ttm_resource_manager directly as parameter here
>>> instead
>>> of the bdev and avoid the whole handling with the macro and global
>>> arrays.
>> We could but that does change the behaviour. I was trying to preserve
>> the old semantics. Because the lifetimes of driver specific managers
>> are not handled by ttm, there's nothing preventing the drivers from,
>> e.g. during reset, deleting the old and setting up new resource
>> managers. By always using ttm_manager_type() we make sure that we're
>> always using the current one. Passing ttm_resource_manager directly
>> makes it impossible to validate that at _show() time
>> ttm_resource_manager is still valid. It's not a problem for vmwgfx
>> because we never reset the resource managers but I didn't feel
>> comfortable changing the semantics like that for all drivers. It could
>> lead to weird crashes, but if you prefer that approach I'm happy to
>> change it.
> Uh resetting resource managers during the lifetime of a drm_device sounds
> very broken. I guess it's somewhat ok over suspend/resume or so when
> userspace cannot access the driver, but even then it smells fishy.
>
> Unless we have a driver doing that I don't think this is a use-case we
> should support.

Yes, completely agree.

We often use the placement enum only because it used to be a flag and 
I'm working on to get rid of that and use a pointer to the manager where 
an allocation comes from directly.

Regards,
Christian.

> -Daniel