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@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.
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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
