On Thu, Feb 09, 2023 at 03:06:10PM +0100, Christian König wrote: > Am 09.02.23 um 14:06 schrieb Maíra Canal: > > On 2/9/23 09:13, Christian König wrote: > > > Am 09.02.23 um 12:23 schrieb Maíra Canal: > > > > On 2/9/23 05:18, Christian König wrote: > > > > > Hello everyone, > > > > > > > > > > the drm_debugfs has a couple of well known design problems. > > > > > > > > > > Especially it wasn't possible to add files between > > > > > initializing and registering > > > > > of DRM devices since the underlying debugfs directory wasn't > > > > > created yet. > > > > > > > > > > The resulting necessity of the driver->debugfs_init() > > > > > callback function is a > > > > > mid-layering which is really frowned on since it creates a horrible > > > > > driver->DRM->driver design layering. > > > > > > > > > > The recent patch "drm/debugfs: create device-centered > > > > > debugfs functions" tried > > > > > to address those problem, but doesn't seem to work > > > > > correctly. This looks like > > > > > a misunderstanding of the call flow around > > > > > drm_debugfs_init(), which is called > > > > > multiple times, once for the primary and once for the render node. > > > > > > > > > > So what happens now is the following: > > > > > > > > > > 1. drm_dev_init() initially allocates the drm_minor objects. > > > > > 2. ... back to the driver ... > > > > > 3. drm_dev_register() is called. > > > > > > > > > > 4. drm_debugfs_init() is called for the primary node. > > > > > 5. drm_framebuffer_debugfs_init(), drm_client_debugfs_init() and > > > > > drm_atomic_debugfs_init() call drm_debugfs_add_file(s)() > > > > > to add the files > > > > > for the primary node. > > > > > 6. The driver->debugfs_init() callback is called to add > > > > > debugfs files for the > > > > > primary node. > > > > > 7. The added files are consumed and added to the primary > > > > > node debugfs directory. > > > > > > > > > > 8. drm_debugfs_init() is called for the render node. > > > > > 9. drm_framebuffer_debugfs_init(), drm_client_debugfs_init() and > > > > > drm_atomic_debugfs_init() call drm_debugfs_add_file(s)() > > > > > to add the files > > > > > again for the render node. > > > > > 10. The driver->debugfs_init() callback is called to add > > > > > debugfs files for the > > > > > render node. > > > > > 11. The added files are consumed and added to the render > > > > > node debugfs directory. > > > > > > > > > > 12. Some more files are added through drm_debugfs_add_file(). > > > > > 13. drm_debugfs_late_register() add the files once more to > > > > > the primary node > > > > > debugfs directory. > > > > > 14. From this point on files added through > > > > > drm_debugfs_add_file() are simply ignored. > > > > > 15. ... back to the driver ... > > > > > > > > > > Because of this the dev->debugfs_mutex lock is also > > > > > completely pointless since > > > > > any concurrent use of the interface would just randomly > > > > > either add the files to > > > > > the primary or render node or just not at all. > > > > > > > > > > Even worse is that this implementation nails the coffin for > > > > > removing the > > > > > driver->debugfs_init() mid-layering because otherwise > > > > > drivers can't control > > > > > where their debugfs (primary/render node) are actually added. > > > > > > > > > > This patch set here now tries to clean this up a bit, but > > > > > most likely isn't > > > > > fully complete either since I didn't audit every driver/call path. > > > > > > > > I tested the patchset on the v3d, vc4 and vkms and all the files > > > > are generated > > > > as expected, but I'm getting the following errors on dmesg: > > > > > > > > [ 3.872026] debugfs: File 'v3d_ident' in directory '0' > > > > already present! > > > > [ 3.872064] debugfs: File 'v3d_ident' in directory '128' > > > > already present! > > > > [ 3.872078] debugfs: File 'v3d_regs' in directory '0' already > > > > present! > > > > [ 3.872087] debugfs: File 'v3d_regs' in directory '128' > > > > already present! > > > > [ 3.872097] debugfs: File 'measure_clock' in directory '0' > > > > already present! > > > > [ 3.872105] debugfs: File 'measure_clock' in directory '128' > > > > already present! > > > > [ 3.872116] debugfs: File 'bo_stats' in directory '0' already > > > > present! > > > > [ 3.872124] debugfs: File 'bo_stats' in directory '128' > > > > already present! > > > > > > > > It looks like the render node is being added twice, since this > > > > doesn't happen > > > > for vc4 and vkms. > > > > > > Thanks for the feedback and yes that's exactly what I meant with > > > that I haven't looked into all code paths. > > > > > > Could it be that v3d registers it's debugfs files from the > > > debugfs_init callback? > > > > Although this is true, I'm not sure if this is the reason why the files > > are > > being registered twice, as this doesn't happen to vc4, and it also uses > > the > > debugfs_init callback. I believe it is somewhat related to the fact that > > v3d is the primary node and the render node. > > I see. Thanks for the hint. > > > > > Best Regards, > > - Maíra Canal > > > > > > > > One alternative would be to just completely nuke support for > > > separate render node debugfs files and only add a symlink to the > > > primary node. Opinions? > > What do you think of this approach? I can't come up with any reason why we > should have separate debugfs files for render nodes and I think it is pretty > much the same reason you came up with the patch for per device debugfs files > instead of per minor. Yeah I think best is to symlink around a bit for compat. I thought we where doing that already, and you can't actually create debugfs files on render nodes? Or did I only dream about this? -Daniel > > Regards, > Christian. > > > > > > > Regards, > > > Christian. > > > > > > > > > > > Otherwise, the patchset looks good to me, but maybe Daniel has > > > > some other > > > > thoughts about it. > > > > > > > > Best Regards, > > > > - Maíra Canal > > > > > > > > > > > > > > Please comment/discuss. > > > > > > > > > > Cheers, > > > > > Christian. > > > > > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
