Try to address the drm_debugfs issues

[Christian König]

Am 14.02.23 um 09:59 schrieb Stanislaw Gruszka:
> On Thu, Feb 09, 2023 at 09:18:35AM +0100, 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.
>>
>> Please comment/discuss.
> What is end goal here regarding debugfs in DRM ? My undersigning is that
> the direction is get rid of debugfs_init callback as described in:
> https://cgit.freedesktop.org/drm/drm-misc/tree/Documentation/gpu/todo.rst#n511
> and also make it driver/device-centric instead of minor-centric as
> described here:
> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=99845faae7099cd704ebf67514c1157c26960a	

Well my main goal is to get rid of the debugfs_init() mid-layering in 
the mid term, everything else is just nice to have.

> I'm asking from accel point of view. We can make things there as they
> should look like at the end for DRM, since currently no drivers have
> established their interfaces and they can be changed.
>
> Is drivers/device-centric mean we should use drm_dev->unique for debugfs
> dir entry name instead of minor ?

Oh, good idea! That would also finally make it a bit less problematic to 
figure out which PCI or platform device corresponds to which debugfs 
directory.

Only potential problem I see is that we would need to rename the 
directory should a driver every decide to set drm_dev->unique to 
something else than the default. But a quick check shows no users of 
drm_dev_set_unique(), so we could potentially just unexport the function

> Or perhaps we should have 2 separate dir entries: one (old dri/minor/)
> for device drm debugfs files and other one for driver specific files ?

How about we just create symlinks between the old and the new directory 
for now which we remove after everything has settled again?

> Also what regarding sysfs ? Should we do something with accel_sysfs_device_minor ?

I see sysfs as a different and probably even more complicated topic.

Regards,
Christian.

>
> Regards
> Stanislaw