Try to address the drm_debugfs issues

[Christian König]

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?

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?

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.
>>
>>