On Thu, Feb 16, 2023 at 07:08:49PM +0200, Jani Nikula wrote:
> On Thu, 16 Feb 2023, Christian König <christian.koenig@xxxxxxx> wrote:
> > Am 16.02.23 um 17:46 schrieb Jani Nikula:
> >> On Thu, 16 Feb 2023, Christian König <christian.koenig@xxxxxxx> wrote:
> >>> Am 16.02.23 um 12:33 schrieb Daniel Vetter:
> >>>> On Thu, Feb 09, 2023 at 09:18:38AM +0100, Christian König wrote:
> >>>>> The mutex was completely pointless in the first place since any
> >>>>> parallel adding of files to this list would result in random
> >>>>> behavior since the list is filled and consumed multiple times.
> >>>>>
> >>>>> Completely drop that approach and just create the files directly.
> >>>>>
> >>>>> This also re-adds the debugfs files to the render node directory and
> >>>>> removes drm_debugfs_late_register().
> >>>>>
> >>>>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> >>>>> ---
> >>>>> drivers/gpu/drm/drm_debugfs.c | 32 +++++++------------------------
> >>>>> drivers/gpu/drm/drm_drv.c | 3 ---
> >>>>> drivers/gpu/drm/drm_internal.h | 5 -----
> >>>>> drivers/gpu/drm/drm_mode_config.c | 2 --
> >>>>> include/drm/drm_device.h | 15 ---------------
> >>>>> 5 files changed, 7 insertions(+), 50 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> >>>>> index 558e3a7271a5..a40288e67264 100644
> >>>>> --- a/drivers/gpu/drm/drm_debugfs.c
> >>>>> +++ b/drivers/gpu/drm/drm_debugfs.c
> >>>>> @@ -246,31 +246,9 @@ void drm_debugfs_dev_register(struct drm_device *dev)
> >>>>> void drm_debugfs_minor_register(struct drm_minor *minor)
> >>>>> {
> >>>>> struct drm_device *dev = minor->dev;
> >>>>> - struct drm_debugfs_entry *entry, *tmp;
> >>>>>
> >>>>> if (dev->driver->debugfs_init)
> >>>>> dev->driver->debugfs_init(minor);
> >>>>> -
> >>>>> - list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> >>>>> - debugfs_create_file(entry->file.name, 0444,
> >>>>> - minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> >>>>> - list_del(&entry->list);
> >>>>> - }
> >>>>> -}
> >>>>> -
> >>>>> -void drm_debugfs_late_register(struct drm_device *dev)
> >>>>> -{
> >>>>> - struct drm_minor *minor = dev->primary;
> >>>>> - struct drm_debugfs_entry *entry, *tmp;
> >>>>> -
> >>>>> - if (!minor)
> >>>>> - return;
> >>>>> -
> >>>>> - list_for_each_entry_safe(entry, tmp, &dev->debugfs_list, list) {
> >>>>> - debugfs_create_file(entry->file.name, 0444,
> >>>>> - minor->debugfs_root, entry, &drm_debugfs_entry_fops);
> >>>>> - list_del(&entry->list);
> >>>>> - }
> >>>>> }
> >>>>>
> >>>>> int drm_debugfs_remove_files(const struct drm_info_list *files, int count,
> >>>>> @@ -343,9 +321,13 @@ void drm_debugfs_add_file(struct drm_device *dev, const char *name,
> >>>>> entry->file.data = data;
> >>>>> entry->dev = dev;
> >>>>>
> >>>>> - mutex_lock(&dev->debugfs_mutex);
> >>>>> - list_add(&entry->list, &dev->debugfs_list);
> >>>>> - mutex_unlock(&dev->debugfs_mutex);
> >>>>> + debugfs_create_file(name, 0444, dev->primary->debugfs_root, entry,
> >>>>> + &drm_debugfs_entry_fops);
> >>>>> +
> >>>>> + /* TODO: This should probably only be a symlink */
> >>>>> + if (dev->render)
> >>>>> + debugfs_create_file(name, 0444, dev->render->debugfs_root,
> >>>>> + entry, &drm_debugfs_entry_fops);
> >>>> Nope. You are fundamentally missing the point of all this, which is:
> >>>>
> >>>> - drivers create debugfs files whenever they want to, as long as it's
> >>>> _before_ drm_dev_register is called.
> >>>>
> >>>> - drm_dev_register will set them all up.
> >>>>
> >>>> This is necessary because otherwise you have the potential for some nice
> >>>> oops and stuff when userspace tries to access these files before the
> >>>> driver is ready.
> >>>>
> >>>> Note that with sysfs all this infrastructure already exists, which is why
> >>>> you can create sysfs files whenever you feel like, and things wont go
> >>>> boom.
> >>> Well Yeah I've considered that, I just don't think it's a good idea for
> >>> debugfs.
> >>>
> >>> debugfs is meant to be a helper for debugging things and that especially
> >>> includes the time between drm_dev_init() and drm_dev_register() because
> >>> that's where we probe the hardware and try to get it working.
> >>>
> >>> Not having the debugfs files which allows for things like hardware
> >>> register access and reading internal state during that is a really and I
> >>> mean REALLY bad idea. This is essentially what we have those files for.
> >> So you mean you want to have early debugfs so you can have some script
> >> hammering the debugfs to get info out between init and register during
> >> probe?
> >
> > Well not hammering. What we usually do in bringup is to set firmware
> > timeout to infinity and the driver then sits and waits for the hw.
> >
> > The tool used to access registers then goes directly through the PCI bar
> > at the moment, but that's essentially a bad idea for registers which you
> > grab a lock for to access (like index/data).
> >
> >>
> >> I just think registering debugfs before everything is ready is a recipe
> >> for disaster. All of the debugfs needs to check all the conditions that
> >> they need across all of the probe stages. It'll be difficult to get it
> >> right. And you'll get cargo culted checks copy pasted all over the
> >> place.
> >
> > Yeah, but it's debugfs. That is not supposed to work under all conditions.
> >
> > Just try to read amdgpu_regs on a not existing register index. This will
> > just hang or reboot your box immediately on APUs.
>
> I'm firmly in the camp that debugfs does not need to work under all
> conditions, but that it must fail gracefully instead of crashing.
Yeah I mean once we talk bring-up, you can just hand-roll the necessary
bring debugfs things that you need to work before the driver is ready to
do anything.
But bring-up debugfs fun is rather special, same way pre-silicon support
tends to be rather special. Shipping that in distros does not sound like a
good idea at all to me.
-Daniel
>
>
> BR,
> Jani.
>
>
> >
> > Regards,
> > Christian.
> >
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>>> So yeah we need the list.
> >>>>
> >>>> This also means that we really should not create the debugfs directories
> >>>> _before_ drm_dev_register is called. That's just fundamentally not how
> >>>> device interface setup should work:
> >>>>
> >>>> 1. you allocate stucts and stuff
> >>>> 2. you fully init everything
> >>>> 3. you register interfaces so they become userspace visible
> >>> How about we create the debugfs directory early and only delay the files
> >>> registered through this drm_debugfs interface until registration time?
> >>>
> >>> This way drivers can still decide if they want the files available
> >>> immediately or only after registration.
> >>>
> >>> What drivers currently do is like radeon setting an accel_working flag
> >>> and registering anyway even if halve the hardware doesn't work.
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>> -Daniel
> >>>>
> >>>>> }
> >>>>> EXPORT_SYMBOL(drm_debugfs_add_file);
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >>>>> index 2cbe028e548c..e7b88b65866c 100644
> >>>>> --- a/drivers/gpu/drm/drm_drv.c
> >>>>> +++ b/drivers/gpu/drm/drm_drv.c
> >>>>> @@ -597,7 +597,6 @@ static void drm_dev_init_release(struct drm_device *dev, void *res)
> >>>>> mutex_destroy(&dev->clientlist_mutex);
> >>>>> mutex_destroy(&dev->filelist_mutex);
> >>>>> mutex_destroy(&dev->struct_mutex);
> >>>>> - mutex_destroy(&dev->debugfs_mutex);
> >>>>> drm_legacy_destroy_members(dev);
> >>>>> }
> >>>>>
> >>>>> @@ -638,14 +637,12 @@ static int drm_dev_init(struct drm_device *dev,
> >>>>> INIT_LIST_HEAD(&dev->filelist_internal);
> >>>>> INIT_LIST_HEAD(&dev->clientlist);
> >>>>> INIT_LIST_HEAD(&dev->vblank_event_list);
> >>>>> - INIT_LIST_HEAD(&dev->debugfs_list);
> >>>>>
> >>>>> spin_lock_init(&dev->event_lock);
> >>>>> mutex_init(&dev->struct_mutex);
> >>>>> mutex_init(&dev->filelist_mutex);
> >>>>> mutex_init(&dev->clientlist_mutex);
> >>>>> mutex_init(&dev->master_mutex);
> >>>>> - mutex_init(&dev->debugfs_mutex);
> >>>>>
> >>>>> ret = drmm_add_action_or_reset(dev, drm_dev_init_release, NULL);
> >>>>> if (ret)
> >>>>> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> >>>>> index 5ff7bf88f162..e215d00ba65c 100644
> >>>>> --- a/drivers/gpu/drm/drm_internal.h
> >>>>> +++ b/drivers/gpu/drm/drm_internal.h
> >>>>> @@ -188,7 +188,6 @@ int drm_debugfs_init(struct drm_minor *minor, int minor_id,
> >>>>> void drm_debugfs_dev_register(struct drm_device *dev);
> >>>>> void drm_debugfs_minor_register(struct drm_minor *minor);
> >>>>> void drm_debugfs_cleanup(struct drm_minor *minor);
> >>>>> -void drm_debugfs_late_register(struct drm_device *dev);
> >>>>> void drm_debugfs_connector_add(struct drm_connector *connector);
> >>>>> void drm_debugfs_connector_remove(struct drm_connector *connector);
> >>>>> void drm_debugfs_crtc_add(struct drm_crtc *crtc);
> >>>>> @@ -205,10 +204,6 @@ static inline void drm_debugfs_cleanup(struct drm_minor *minor)
> >>>>> {
> >>>>> }
> >>>>>
> >>>>> -static inline void drm_debugfs_late_register(struct drm_device *dev)
> >>>>> -{
> >>>>> -}
> >>>>> -
> >>>>> static inline void drm_debugfs_connector_add(struct drm_connector *connector)
> >>>>> {
> >>>>> }
> >>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> >>>>> index 87eb591fe9b5..8525ef851540 100644
> >>>>> --- a/drivers/gpu/drm/drm_mode_config.c
> >>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
> >>>>> @@ -54,8 +54,6 @@ int drm_modeset_register_all(struct drm_device *dev)
> >>>>> if (ret)
> >>>>> goto err_connector;
> >>>>>
> >>>>> - drm_debugfs_late_register(dev);
> >>>>> -
> >>>>> return 0;
> >>>>>
> >>>>> err_connector:
> >>>>> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> >>>>> index 7cf4afae2e79..900ad7478dd8 100644
> >>>>> --- a/include/drm/drm_device.h
> >>>>> +++ b/include/drm/drm_device.h
> >>>>> @@ -311,21 +311,6 @@ struct drm_device {
> >>>>> */
> >>>>> struct drm_fb_helper *fb_helper;
> >>>>>
> >>>>> - /**
> >>>>> - * @debugfs_mutex:
> >>>>> - *
> >>>>> - * Protects &debugfs_list access.
> >>>>> - */
> >>>>> - struct mutex debugfs_mutex;
> >>>>> -
> >>>>> - /**
> >>>>> - * @debugfs_list:
> >>>>> - *
> >>>>> - * List of debugfs files to be created by the DRM device. The files
> >>>>> - * must be added during drm_dev_register().
> >>>>> - */
> >>>>> - struct list_head debugfs_list;
> >>>>> -
> >>>>> /* Everything below here is for legacy driver, never use! */
> >>>>> /* private: */
> >>>>> #if IS_ENABLED(CONFIG_DRM_LEGACY)
> >>>>> --
> >>>>> 2.34.1
> >>>>>
> >
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
