Try to address the drm_debugfs issues

[Christian König]

Am 16.02.23 um 12:34 schrieb Daniel Vetter:
> 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?

No, we still have that distinction around unfortunately.

That's why this went boom for me in the first place.

Christian.

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