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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Cc: Christian Koenig <christian.koenig@xxxxxxx>
> > Cc: Huang Rui <ray.huang@xxxxxxx>
> > Cc: David Airlie <airlied@xxxxxxxx>
> > Cc: Daniel Vetter <daniel@xxxxxxxx>
>
> 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.
z
