Hi Andy,
Am Mittwoch, 11. Dezember 2024, 08:07:21 CET schrieb Andy Yan:
> At 2024-12-10 19:57:44, "Heiko Stübner" <heiko@xxxxxxxxx> wrote:
> >Am Montag, 9. Dezember 2024, 13:29:13 CET schrieb Andy Yan:
> >> From: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
> >>
> >> /sys/kernel/debug/dri/vop2/summary: dump vop display state
> >> /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
> >> /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
> >> activated modules
> >>
> >> Reviewed-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> >> Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
> >> Tested-by: Michael Riesch <michael.riesch@xxxxxxxxxxxxxx> # on RK3568
> >> Tested-by: Detlev Casanova <detlev.casanova@xxxxxxxxxxxxx>
> >> ---
> >
> >> +static void __vop2_regs_dump(struct seq_file *s, bool active_only)
> >> +{
> >> + struct drm_info_node *node = s->private;
> >> + struct vop2 *vop2 = node->info_ent->data;
> >> + struct drm_minor *minor = node->minor;
> >> + struct drm_device *drm_dev = minor->dev;
> >> + const struct vop2_regs_dump *dump;
> >> + unsigned int i;
> >> +
> >> + drm_modeset_lock_all(drm_dev);
> >> +
> >> + regcache_drop_region(vop2->map, 0, vop2_regmap_config.max_register);
> >> +
> >> + if (vop2->enable_count) {
> >> + for (i = 0; i < vop2->data->regs_dump_size; i++) {
> >> + dump = &vop2->data->regs_dump[i];
> >> + vop2_regs_print(vop2, s, dump, active_only);
> >> + }
> >> + } else {
> >> + seq_printf(s, "VOP disabled\n");
> >> + }
> >> + drm_modeset_unlock_all(drm_dev);
> >> +
> >
> >nit: not needed empty line at the end of the function
>
> Will fixed in V6.
>
> >
> >> +}
> >> +
> >
> >> +static void vop2_debugfs_init(struct vop2 *vop2, struct drm_minor *minor)
> >> +{
> >> + struct dentry *root;
> >> + unsigned int i;
> >> +
> >> + root = debugfs_create_dir("vop2", minor->debugfs_root);
> >> + if (!IS_ERR(root)) {
> >> + for (i = 0; i < ARRAY_SIZE(vop2_debugfs_list); i++)
> >> + vop2_debugfs_list[i].data = vop2;
> >> +
> >> + drm_debugfs_create_files(vop2_debugfs_list,
> >> + ARRAY_SIZE(vop2_debugfs_list),
> >> + root, minor);
> >> + }
> >> +}
> >> +
> >> +static int vop2_crtc_late_register(struct drm_crtc *crtc)
> >> +{
> >> + struct vop2_video_port *vp = to_vop2_video_port(crtc);
> >> + struct vop2 *vop2 = vp->vop2;
> >> +
> >> + if (drm_crtc_index(crtc) == 0)
> >> + vop2_debugfs_init(vop2, crtc->dev->primary);
> >> +
> >> + return 0;
> >> +}
> >
> >I'm wondering about, shoudln't there be an unregister step too?
> >I.e. the late_register callback says:
> >"should be unregistered in the early_unregister callback" [0].
> >
> >And there exists drm_debugfs_remove_files(), though it doesn't
> >seem t be used much - just by tegra.
> >
> >I haven't managed to find drm code handling that automatically though?
>
> Did a little digging：
> rockchip_drm_unbind --》drm_dev_unregister(drm_dev)--》drm_debugfs_dev_fini
> --》debugfs_remove_recursive(dev->debugfs_root);
> This will remove all the debugfs file under /dri/0-->display-subsystem when we
> remove rockchipdrm when it build as module.
thanks a lot for finding this. And yes, that covers all possible cases.
> If it is builtin, whether we remove them seems to make no difference when we
> reboot or thutdown the system ?
> I also searched for other platforms using similar late register hook, and it seems they haven't remove debugfs either.
unbind can be called even if the driver is builtin - i.e. a driver
does not need to be a module to be unbount/rebound again.
But the drm_debugfs_dev_fini() function you found, solves that
in all cases, so this patch can stay as it is - minus the empty line.
Thanks a lot
Heiko
