Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Mon, Jan 30, 2017 at 10:10:15AM +0100, Thierry Reding wrote: > On Mon, Jan 30, 2017 at 10:03:44AM +0100, Daniel Vetter wrote: > > On Mon, Jan 30, 2017 at 09:58:48AM +0100, Thierry Reding wrote: > > > On Fri, Jan 27, 2017 at 03:05:46PM +0100, Daniel Vetter wrote: > > > > On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote: > > > > > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > > > > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > > > > > This patchset removes the need for drivers to clean up their > > > > > > > debugfs > > > > > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > > > > > This funtion is also called should the driver error out in it's > > > > > > > drm_driver.debugfs_init callback. > > > > > > > > > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > > > > > - tegra in it's connectors, not sure if I can remove it. > > > > > > > > > > > > I read through them, and they're removed on the component device > > > > > > nodes > > > > > > stuff. That looks somewhat fishy from a lifetime point of view, and > > > > > > I > > > > > > think removing all that code would be better, too. > > > > > > > > > > What makes you think that's problematic from a lifetime point of view? > > > > > The component device is tied to the DRM device, so these callbacks are > > > > > called at the right time. > > > > > > > > debugfs is a userspace interface, which should disappear when > > > > drm_dev_unregister gets called. I'm not sure at all whether that lines > > > > up > > > > with the cleanup of all your component nodes, but otoh it's rather > > > > academic since you can't hotplug a tegra. > > > > > > > > > That said, I think it's safe to remove the other debugfs files from > > > > > Tegra. It might not be possible to remove the cleanup functions > > > > > altogether, though, because they have to do a special dance involving > > > > > kmemdup() drm_debugfs_create_files() and kfree() in order to support > > > > > debugfs files for multiple instances of subdevices. > > > > > > > > Hm, that entire "do debugfs on the minor" thing makes almost never > > > > sense. > > > > All the things we have left in modern drivers are either per-fd, or > > > > per-device. Nothing of interest is per-minor. Or do you mean something > > > > else? > > > > > > I'm not sure I understand what you're saying. We have plenty of code > > > that adds debugfs files to the connector's debugfs entry. And that's > > > within the minor's debugfs root. > > > > > > Am I missing something? > > > > Per-connector entries are fine, per-minor imo not. > > Most, if not all, debugfs files in Tegra a per-connector. We have a > couple that are per-CRTC. And then we have two files that are on the > minor, which is something I had copied from i915, if I remember > correctly, though I can't seem to find the original anymore. Maybe > that was moved somewhere else in the meantime? All the code I've found in tegra about debugfs is per drm_minor. Some of it create subdirectories within that, but nothing uses the crtc and connector ->debugfs_root stuff (which is only in the primary drm_minor debugfs directory). > > This is a historical accident, but it also doesn't really hurt anyone. > > I think it'd make much more sense to move everything into a > > per-devices entry (with maybe backwards compat links from minor to > > devices). > > With per-device entries you mean rooted at the device backing the CRTC, > encoder, connector, ...? I didn't see any drm support for encoders, no idea what you mean. > > But really, this is 100% orthogonal to the cleanup here. > > If we want to get rid of the remainder of the cleanup, then it's not > entirely orthogonal anymore. =) > > Not to say that this cleanup isn't useful in its own right. Well, looking at this a bit more we might go even further by using debugfs_remove_recursive(), then we could remove even the tegra stuff. Atm I think that's not doable because tegra creates its own subdirectories in drm_minor->debugfs_root. I guess that's a mess for a different day though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
Den 01.03.2017 15.31, skrev Daniel Vetter: On Fri, Jan 27, 2017 at 03:29:29PM +0100, Daniel Vetter wrote: On Fri, Jan 27, 2017 at 03:23:43PM +0100, Noralf Trønnes wrote: Den 27.01.2017 08.49, skrev Daniel Vetter: On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: This patchset removes the need for drivers to clean up their debugfs files on exit. It is done automatically in drm_debugfs_cleanup(). This funtion is also called should the driver error out in it's drm_driver.debugfs_init callback. Two drivers still use drm_debugfs_remove_files(): - tegra in it's connectors, not sure if I can remove it. I read through them, and they're removed on the component device nodes stuff. That looks somewhat fishy from a lifetime point of view, and I think removing all that code would be better, too. - qxl because it's debugfs files list is part of struct qxl_device which is freed on unload before drm_minor_unregister() is called cleaning debugfs. In -next qxl is already demidlayered and there's no longer an unload hook. This should be all safe ... why is it not? My bad, I fetched linux-next a few days ago and figured it was up-to-date-enough. Duh, I should have known better after following drm for a year now, it is constantly changing, no pauses. Can you please ping me when you have pulled driver patches and I'll respin msm, tegra and qxl (and others if necessary), and remove the hook. It's much easier for me to do a small patchset, it's really a strain to get everything lined up correctly with big changes. I didn't have that patch juggling class when in school, so I'm late to the game :-) You're doing great with the patch juggling :-) I've just made a pass through the series and merge what's already reviewed/acked. Ok, pulled in the remaining patch (I hope, pls ping if I missed one). We have only a few debugfs_remove calls left, and I think it's safe to remove them all too. Up to do that too? Then we could remove the export, to make sure no new driver gets this wrong ... Yes, they're all in now. I'll fix up what's left. Noralf. Thanks a lot, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Fri, Jan 27, 2017 at 03:29:29PM +0100, Daniel Vetter wrote: > On Fri, Jan 27, 2017 at 03:23:43PM +0100, Noralf Trønnes wrote: > > > > Den 27.01.2017 08.49, skrev Daniel Vetter: > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > > This patchset removes the need for drivers to clean up their debugfs > > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > > This funtion is also called should the driver error out in it's > > > > drm_driver.debugfs_init callback. > > > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > > - tegra in it's connectors, not sure if I can remove it. > > > I read through them, and they're removed on the component device nodes > > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > > think removing all that code would be better, too. > > > > > > > - qxl because it's debugfs files list is part of struct qxl_device which > > > >is freed on unload before drm_minor_unregister() is called cleaning > > > > debugfs. > > > In -next qxl is already demidlayered and there's no longer an unload hook. > > > This should be all safe ... why is it not? > > > > My bad, I fetched linux-next a few days ago and figured it was > > up-to-date-enough. Duh, I should have known better after following drm for > > a year now, it is constantly changing, no pauses. > > > > Can you please ping me when you have pulled driver patches and I'll respin > > msm, tegra and qxl (and others if necessary), and remove the hook. > > It's much easier for me to do a small patchset, it's really a strain to get > > everything lined up correctly with big changes. I didn't have that patch > > juggling class when in school, so I'm late to the game :-) > > You're doing great with the patch juggling :-) I've just made a pass > through the series and merge what's already reviewed/acked. Ok, pulled in the remaining patch (I hope, pls ping if I missed one). We have only a few debugfs_remove calls left, and I think it's safe to remove them all too. Up to do that too? Then we could remove the export, to make sure no new driver gets this wrong ... Thanks a lot, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Mon, Jan 30, 2017 at 10:03:44AM +0100, Daniel Vetter wrote: > On Mon, Jan 30, 2017 at 09:58:48AM +0100, Thierry Reding wrote: > > On Fri, Jan 27, 2017 at 03:05:46PM +0100, Daniel Vetter wrote: > > > On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote: > > > > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > > > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > > > > This patchset removes the need for drivers to clean up their debugfs > > > > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > > > > This funtion is also called should the driver error out in it's > > > > > > drm_driver.debugfs_init callback. > > > > > > > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > > > > - tegra in it's connectors, not sure if I can remove it. > > > > > > > > > > I read through them, and they're removed on the component device nodes > > > > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > > > > think removing all that code would be better, too. > > > > > > > > What makes you think that's problematic from a lifetime point of view? > > > > The component device is tied to the DRM device, so these callbacks are > > > > called at the right time. > > > > > > debugfs is a userspace interface, which should disappear when > > > drm_dev_unregister gets called. I'm not sure at all whether that lines up > > > with the cleanup of all your component nodes, but otoh it's rather > > > academic since you can't hotplug a tegra. > > > > > > > That said, I think it's safe to remove the other debugfs files from > > > > Tegra. It might not be possible to remove the cleanup functions > > > > altogether, though, because they have to do a special dance involving > > > > kmemdup() drm_debugfs_create_files() and kfree() in order to support > > > > debugfs files for multiple instances of subdevices. > > > > > > Hm, that entire "do debugfs on the minor" thing makes almost never sense. > > > All the things we have left in modern drivers are either per-fd, or > > > per-device. Nothing of interest is per-minor. Or do you mean something > > > else? > > > > I'm not sure I understand what you're saying. We have plenty of code > > that adds debugfs files to the connector's debugfs entry. And that's > > within the minor's debugfs root. > > > > Am I missing something? > > Per-connector entries are fine, per-minor imo not. Most, if not all, debugfs files in Tegra a per-connector. We have a couple that are per-CRTC. And then we have two files that are on the minor, which is something I had copied from i915, if I remember correctly, though I can't seem to find the original anymore. Maybe that was moved somewhere else in the meantime? > This is a historical accident, but it also doesn't really hurt anyone. > I think it'd make much more sense to move everything into a > per-devices entry (with maybe backwards compat links from minor to > devices). With per-device entries you mean rooted at the device backing the CRTC, encoder, connector, ...? > But really, this is 100% orthogonal to the cleanup here. If we want to get rid of the remainder of the cleanup, then it's not entirely orthogonal anymore. =) Not to say that this cleanup isn't useful in its own right. Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Mon, Jan 30, 2017 at 09:58:48AM +0100, Thierry Reding wrote: > On Fri, Jan 27, 2017 at 03:05:46PM +0100, Daniel Vetter wrote: > > On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote: > > > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > > > This patchset removes the need for drivers to clean up their debugfs > > > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > > > This funtion is also called should the driver error out in it's > > > > > drm_driver.debugfs_init callback. > > > > > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > > > - tegra in it's connectors, not sure if I can remove it. > > > > > > > > I read through them, and they're removed on the component device nodes > > > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > > > think removing all that code would be better, too. > > > > > > What makes you think that's problematic from a lifetime point of view? > > > The component device is tied to the DRM device, so these callbacks are > > > called at the right time. > > > > debugfs is a userspace interface, which should disappear when > > drm_dev_unregister gets called. I'm not sure at all whether that lines up > > with the cleanup of all your component nodes, but otoh it's rather > > academic since you can't hotplug a tegra. > > > > > That said, I think it's safe to remove the other debugfs files from > > > Tegra. It might not be possible to remove the cleanup functions > > > altogether, though, because they have to do a special dance involving > > > kmemdup() drm_debugfs_create_files() and kfree() in order to support > > > debugfs files for multiple instances of subdevices. > > > > Hm, that entire "do debugfs on the minor" thing makes almost never sense. > > All the things we have left in modern drivers are either per-fd, or > > per-device. Nothing of interest is per-minor. Or do you mean something > > else? > > I'm not sure I understand what you're saying. We have plenty of code > that adds debugfs files to the connector's debugfs entry. And that's > within the minor's debugfs root. > > Am I missing something? Per-connector entries are fine, per-minor imo not. This is a historical accident, but it also doesn't really hurt anyone. I think it'd make much more sense to move everything into a per-devices entry (with maybe backwards compat links from minor to devices). But really, this is 100% orthogonal to the cleanup here. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Fri, Jan 27, 2017 at 03:05:46PM +0100, Daniel Vetter wrote: > On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote: > > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > > This patchset removes the need for drivers to clean up their debugfs > > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > > This funtion is also called should the driver error out in it's > > > > drm_driver.debugfs_init callback. > > > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > > - tegra in it's connectors, not sure if I can remove it. > > > > > > I read through them, and they're removed on the component device nodes > > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > > think removing all that code would be better, too. > > > > What makes you think that's problematic from a lifetime point of view? > > The component device is tied to the DRM device, so these callbacks are > > called at the right time. > > debugfs is a userspace interface, which should disappear when > drm_dev_unregister gets called. I'm not sure at all whether that lines up > with the cleanup of all your component nodes, but otoh it's rather > academic since you can't hotplug a tegra. > > > That said, I think it's safe to remove the other debugfs files from > > Tegra. It might not be possible to remove the cleanup functions > > altogether, though, because they have to do a special dance involving > > kmemdup() drm_debugfs_create_files() and kfree() in order to support > > debugfs files for multiple instances of subdevices. > > Hm, that entire "do debugfs on the minor" thing makes almost never sense. > All the things we have left in modern drivers are either per-fd, or > per-device. Nothing of interest is per-minor. Or do you mean something > else? I'm not sure I understand what you're saying. We have plenty of code that adds debugfs files to the connector's debugfs entry. And that's within the minor's debugfs root. Am I missing something? Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Fri, Jan 27, 2017 at 03:23:43PM +0100, Noralf Trønnes wrote: > > Den 27.01.2017 08.49, skrev Daniel Vetter: > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > This patchset removes the need for drivers to clean up their debugfs > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > This funtion is also called should the driver error out in it's > > > drm_driver.debugfs_init callback. > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > - tegra in it's connectors, not sure if I can remove it. > > I read through them, and they're removed on the component device nodes > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > think removing all that code would be better, too. > > > > > - qxl because it's debugfs files list is part of struct qxl_device which > > >is freed on unload before drm_minor_unregister() is called cleaning > > > debugfs. > > In -next qxl is already demidlayered and there's no longer an unload hook. > > This should be all safe ... why is it not? > > My bad, I fetched linux-next a few days ago and figured it was > up-to-date-enough. Duh, I should have known better after following drm for > a year now, it is constantly changing, no pauses. > > Can you please ping me when you have pulled driver patches and I'll respin > msm, tegra and qxl (and others if necessary), and remove the hook. > It's much easier for me to do a small patchset, it's really a strain to get > everything lined up correctly with big changes. I didn't have that patch > juggling class when in school, so I'm late to the game :-) You're doing great with the patch juggling :-) I've just made a pass through the series and merge what's already reviewed/acked. Thanks, Daniel > > > Noralf. > > > > Daniel, > > > I was unable to remove the drm_driver.debugfs_cleanup hook completely, > > > since drm/msm uses it to free memory. Maybe it can be freed elsewhere. > > Well this is definitely a massive step into a good direction, great work. > > For msm I think we can just move the two calls left in msm_debugfs_cleanup > > in msm_drm_uninit, right after the call to drm_dev_unregister. > > -Daniel > > > > > > > > Note: > > > The driver patches are only compile tested. > > > > > > > > > Noralf. > > > > > > > > > Noralf Trønnes (19): > > >drm: debugfs: Remove all files automatically on cleanup > > >drm: drm_minor_register(): Clean up debugfs on failure > > >drm/atomic: Remove drm_atomic_debugfs_cleanup() > > >drm/amd/amdgpu: Remove drm_debugfs_remove_files() call > > >drm/armada: Remove armada_drm_debugfs_cleanup() > > >drm/etnaviv: allow build with COMPILE_TEST > > >drm/etnaviv: Remove etnaviv_debugfs_cleanup() > > >drm/hdlcd: Remove hdlcd_debugfs_cleanup() > > >drm/msm: Remove drm_debugfs_remove_files() calls > > >drm/nouveau: Remove nouveau_drm_debugfs_cleanup() > > >drm/omap: Remove omap_debugfs_cleanup() > > >drm/radeon: Remove drm_debugfs_remove_files() call > > >drm/sti: Remove drm_debugfs_remove_files() calls > > >drm/tegra: Remove tegra_debugfs_cleanup() > > >drm/tilcdc: Remove tilcdc_debugfs_cleanup() > > >drm/vc4: Remove vc4_debugfs_cleanup() > > >drm/virtio: Remove virtio_gpu_debugfs_takedown() > > >drm/qxl: Remove qxl_debugfs_takedown() > > >drm/i915: Remove i915_debugfs_unregister() > > > > > > drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 - > > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 -- > > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 1 - > > > drivers/gpu/drm/arm/hdlcd_drv.c| 7 --- > > > drivers/gpu/drm/armada/armada_debugfs.c| 65 +++- > > > drivers/gpu/drm/armada/armada_drm.h| 1 - > > > drivers/gpu/drm/armada/armada_drv.c| 3 - > > > drivers/gpu/drm/drm_atomic.c | 7 --- > > > drivers/gpu/drm/drm_crtc_internal.h| 1 - > > > drivers/gpu/drm/drm_debugfs.c | 29 + > > > drivers/gpu/drm/drm_drv.c | 2 +- > > > drivers/gpu/drm/etnaviv/Kconfig| 2 +- > > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 7 --- > > > drivers/gpu/drm/i915/i915_debugfs.c| 97 > > > -- > > > drivers/gpu/drm/i915/i915_drv.c| 1 - > > > drivers/gpu/drm/i915/i915_drv.h| 2 - > > > drivers/gpu/drm/i915/intel_drv.h | 1 - > > > drivers/gpu/drm/i915/intel_pipe_crc.c | 68 - > > > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c| 7 --- > > > drivers/gpu/drm/msm/msm_debugfs.c | 2 - > > > drivers/gpu/drm/msm/msm_perf.c | 29 + > > > drivers/gpu/drm/msm/msm_rd.c | 31 +- > > > drivers/gpu/drm/nouveau/nouveau_debugfs.c | 62 --- > > > drivers/gpu/drm/nouveau/nouveau_debugfs.h | 6 -- > > > drivers/gpu/drm/nouveau/nouveau_drm.c | 1 - > > >
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
Den 27.01.2017 08.49, skrev Daniel Vetter: On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: This patchset removes the need for drivers to clean up their debugfs files on exit. It is done automatically in drm_debugfs_cleanup(). This funtion is also called should the driver error out in it's drm_driver.debugfs_init callback. Two drivers still use drm_debugfs_remove_files(): - tegra in it's connectors, not sure if I can remove it. I read through them, and they're removed on the component device nodes stuff. That looks somewhat fishy from a lifetime point of view, and I think removing all that code would be better, too. - qxl because it's debugfs files list is part of struct qxl_device which is freed on unload before drm_minor_unregister() is called cleaning debugfs. In -next qxl is already demidlayered and there's no longer an unload hook. This should be all safe ... why is it not? My bad, I fetched linux-next a few days ago and figured it was up-to-date-enough. Duh, I should have known better after following drm for a year now, it is constantly changing, no pauses. Can you please ping me when you have pulled driver patches and I'll respin msm, tegra and qxl (and others if necessary), and remove the hook. It's much easier for me to do a small patchset, it's really a strain to get everything lined up correctly with big changes. I didn't have that patch juggling class when in school, so I'm late to the game :-) Noralf. Daniel, I was unable to remove the drm_driver.debugfs_cleanup hook completely, since drm/msm uses it to free memory. Maybe it can be freed elsewhere. Well this is definitely a massive step into a good direction, great work. For msm I think we can just move the two calls left in msm_debugfs_cleanup in msm_drm_uninit, right after the call to drm_dev_unregister. -Daniel Note: The driver patches are only compile tested. Noralf. Noralf Trønnes (19): drm: debugfs: Remove all files automatically on cleanup drm: drm_minor_register(): Clean up debugfs on failure drm/atomic: Remove drm_atomic_debugfs_cleanup() drm/amd/amdgpu: Remove drm_debugfs_remove_files() call drm/armada: Remove armada_drm_debugfs_cleanup() drm/etnaviv: allow build with COMPILE_TEST drm/etnaviv: Remove etnaviv_debugfs_cleanup() drm/hdlcd: Remove hdlcd_debugfs_cleanup() drm/msm: Remove drm_debugfs_remove_files() calls drm/nouveau: Remove nouveau_drm_debugfs_cleanup() drm/omap: Remove omap_debugfs_cleanup() drm/radeon: Remove drm_debugfs_remove_files() call drm/sti: Remove drm_debugfs_remove_files() calls drm/tegra: Remove tegra_debugfs_cleanup() drm/tilcdc: Remove tilcdc_debugfs_cleanup() drm/vc4: Remove vc4_debugfs_cleanup() drm/virtio: Remove virtio_gpu_debugfs_takedown() drm/qxl: Remove qxl_debugfs_takedown() drm/i915: Remove i915_debugfs_unregister() drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 - drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 -- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 1 - drivers/gpu/drm/arm/hdlcd_drv.c| 7 --- drivers/gpu/drm/armada/armada_debugfs.c| 65 +++- drivers/gpu/drm/armada/armada_drm.h| 1 - drivers/gpu/drm/armada/armada_drv.c| 3 - drivers/gpu/drm/drm_atomic.c | 7 --- drivers/gpu/drm/drm_crtc_internal.h| 1 - drivers/gpu/drm/drm_debugfs.c | 29 + drivers/gpu/drm/drm_drv.c | 2 +- drivers/gpu/drm/etnaviv/Kconfig| 2 +- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 7 --- drivers/gpu/drm/i915/i915_debugfs.c| 97 -- drivers/gpu/drm/i915/i915_drv.c| 1 - drivers/gpu/drm/i915/i915_drv.h| 2 - drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_pipe_crc.c | 68 - drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c| 7 --- drivers/gpu/drm/msm/msm_debugfs.c | 2 - drivers/gpu/drm/msm/msm_perf.c | 29 + drivers/gpu/drm/msm/msm_rd.c | 31 +- drivers/gpu/drm/nouveau/nouveau_debugfs.c | 62 --- drivers/gpu/drm/nouveau/nouveau_debugfs.h | 6 -- drivers/gpu/drm/nouveau/nouveau_drm.c | 1 - drivers/gpu/drm/omapdrm/omap_debugfs.c | 9 --- drivers/gpu/drm/omapdrm/omap_drv.c | 1 - drivers/gpu/drm/omapdrm/omap_drv.h | 1 - drivers/gpu/drm/qxl/qxl_debugfs.c | 9 --- drivers/gpu/drm/qxl/qxl_drv.c | 1 - drivers/gpu/drm/qxl/qxl_drv.h | 1 - drivers/gpu/drm/radeon/radeon_device.c | 16 - drivers/gpu/drm/sti/sti_drv.c | 48 ++- drivers/gpu/drm/sti/sti_dvo.c | 10 --- drivers/gpu/drm/sti/sti_hda.c | 11 drivers/gpu/drm/sti/sti_hdmi.c | 11 drivers/gpu/drm/sti/sti_tvout.c| 8 --- drivers/gpu/drm/tegra/drm.c
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Fri, Jan 27, 2017 at 10:36:16AM +0100, Thierry Reding wrote: > On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > > This patchset removes the need for drivers to clean up their debugfs > > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > > This funtion is also called should the driver error out in it's > > > drm_driver.debugfs_init callback. > > > > > > Two drivers still use drm_debugfs_remove_files(): > > > - tegra in it's connectors, not sure if I can remove it. > > > > I read through them, and they're removed on the component device nodes > > stuff. That looks somewhat fishy from a lifetime point of view, and I > > think removing all that code would be better, too. > > What makes you think that's problematic from a lifetime point of view? > The component device is tied to the DRM device, so these callbacks are > called at the right time. debugfs is a userspace interface, which should disappear when drm_dev_unregister gets called. I'm not sure at all whether that lines up with the cleanup of all your component nodes, but otoh it's rather academic since you can't hotplug a tegra. > That said, I think it's safe to remove the other debugfs files from > Tegra. It might not be possible to remove the cleanup functions > altogether, though, because they have to do a special dance involving > kmemdup() drm_debugfs_create_files() and kfree() in order to support > debugfs files for multiple instances of subdevices. Hm, that entire "do debugfs on the minor" thing makes almost never sense. All the things we have left in modern drivers are either per-fd, or per-device. Nothing of interest is per-minor. Or do you mean something else? Either way would be nice if we can remove them too, so that drm_debugfs_remove_files could be unexported ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Fri, Jan 27, 2017 at 08:49:34AM +0100, Daniel Vetter wrote: > On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > > This patchset removes the need for drivers to clean up their debugfs > > files on exit. It is done automatically in drm_debugfs_cleanup(). > > This funtion is also called should the driver error out in it's > > drm_driver.debugfs_init callback. > > > > Two drivers still use drm_debugfs_remove_files(): > > - tegra in it's connectors, not sure if I can remove it. > > I read through them, and they're removed on the component device nodes > stuff. That looks somewhat fishy from a lifetime point of view, and I > think removing all that code would be better, too. What makes you think that's problematic from a lifetime point of view? The component device is tied to the DRM device, so these callbacks are called at the right time. That said, I think it's safe to remove the other debugfs files from Tegra. It might not be possible to remove the cleanup functions altogether, though, because they have to do a special dance involving kmemdup() drm_debugfs_create_files() and kfree() in order to support debugfs files for multiple instances of subdevices. Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/19] drm: debugfs: Remove all files automatically on cleanup
On Thu, Jan 26, 2017 at 11:56:02PM +0100, Noralf Trønnes wrote: > This patchset removes the need for drivers to clean up their debugfs > files on exit. It is done automatically in drm_debugfs_cleanup(). > This funtion is also called should the driver error out in it's > drm_driver.debugfs_init callback. > > Two drivers still use drm_debugfs_remove_files(): > - tegra in it's connectors, not sure if I can remove it. I read through them, and they're removed on the component device nodes stuff. That looks somewhat fishy from a lifetime point of view, and I think removing all that code would be better, too. > - qxl because it's debugfs files list is part of struct qxl_device which > is freed on unload before drm_minor_unregister() is called cleaning debugfs. In -next qxl is already demidlayered and there's no longer an unload hook. This should be all safe ... why is it not? > Daniel, > I was unable to remove the drm_driver.debugfs_cleanup hook completely, > since drm/msm uses it to free memory. Maybe it can be freed elsewhere. Well this is definitely a massive step into a good direction, great work. For msm I think we can just move the two calls left in msm_debugfs_cleanup in msm_drm_uninit, right after the call to drm_dev_unregister. -Daniel > > > Note: > The driver patches are only compile tested. > > > Noralf. > > > Noralf Trønnes (19): > drm: debugfs: Remove all files automatically on cleanup > drm: drm_minor_register(): Clean up debugfs on failure > drm/atomic: Remove drm_atomic_debugfs_cleanup() > drm/amd/amdgpu: Remove drm_debugfs_remove_files() call > drm/armada: Remove armada_drm_debugfs_cleanup() > drm/etnaviv: allow build with COMPILE_TEST > drm/etnaviv: Remove etnaviv_debugfs_cleanup() > drm/hdlcd: Remove hdlcd_debugfs_cleanup() > drm/msm: Remove drm_debugfs_remove_files() calls > drm/nouveau: Remove nouveau_drm_debugfs_cleanup() > drm/omap: Remove omap_debugfs_cleanup() > drm/radeon: Remove drm_debugfs_remove_files() call > drm/sti: Remove drm_debugfs_remove_files() calls > drm/tegra: Remove tegra_debugfs_cleanup() > drm/tilcdc: Remove tilcdc_debugfs_cleanup() > drm/vc4: Remove vc4_debugfs_cleanup() > drm/virtio: Remove virtio_gpu_debugfs_takedown() > drm/qxl: Remove qxl_debugfs_takedown() > drm/i915: Remove i915_debugfs_unregister() > > drivers/gpu/drm/amd/amdgpu/amdgpu.h| 1 - > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 -- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 1 - > drivers/gpu/drm/arm/hdlcd_drv.c| 7 --- > drivers/gpu/drm/armada/armada_debugfs.c| 65 +++- > drivers/gpu/drm/armada/armada_drm.h| 1 - > drivers/gpu/drm/armada/armada_drv.c| 3 - > drivers/gpu/drm/drm_atomic.c | 7 --- > drivers/gpu/drm/drm_crtc_internal.h| 1 - > drivers/gpu/drm/drm_debugfs.c | 29 + > drivers/gpu/drm/drm_drv.c | 2 +- > drivers/gpu/drm/etnaviv/Kconfig| 2 +- > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 7 --- > drivers/gpu/drm/i915/i915_debugfs.c| 97 > -- > drivers/gpu/drm/i915/i915_drv.c| 1 - > drivers/gpu/drm/i915/i915_drv.h| 2 - > drivers/gpu/drm/i915/intel_drv.h | 1 - > drivers/gpu/drm/i915/intel_pipe_crc.c | 68 - > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c| 7 --- > drivers/gpu/drm/msm/msm_debugfs.c | 2 - > drivers/gpu/drm/msm/msm_perf.c | 29 + > drivers/gpu/drm/msm/msm_rd.c | 31 +- > drivers/gpu/drm/nouveau/nouveau_debugfs.c | 62 --- > drivers/gpu/drm/nouveau/nouveau_debugfs.h | 6 -- > drivers/gpu/drm/nouveau/nouveau_drm.c | 1 - > drivers/gpu/drm/omapdrm/omap_debugfs.c | 9 --- > drivers/gpu/drm/omapdrm/omap_drv.c | 1 - > drivers/gpu/drm/omapdrm/omap_drv.h | 1 - > drivers/gpu/drm/qxl/qxl_debugfs.c | 9 --- > drivers/gpu/drm/qxl/qxl_drv.c | 1 - > drivers/gpu/drm/qxl/qxl_drv.h | 1 - > drivers/gpu/drm/radeon/radeon_device.c | 16 - > drivers/gpu/drm/sti/sti_drv.c | 48 ++- > drivers/gpu/drm/sti/sti_dvo.c | 10 --- > drivers/gpu/drm/sti/sti_hda.c | 11 > drivers/gpu/drm/sti/sti_hdmi.c | 11 > drivers/gpu/drm/sti/sti_tvout.c| 8 --- > drivers/gpu/drm/tegra/drm.c| 7 --- > drivers/gpu/drm/tilcdc/tilcdc_drv.c| 12 > drivers/gpu/drm/tilcdc/tilcdc_drv.h| 2 - > drivers/gpu/drm/vc4/vc4_debugfs.c | 6 -- > drivers/gpu/drm/vc4/vc4_drv.c | 1 - > drivers/gpu/drm/vc4/vc4_drv.h | 1 - > drivers/gpu/drm/virtio/virtgpu_debugfs.c | 8 --- > drivers/gpu/drm/virtio/virtgpu_drv.c | 1 - > drivers/gpu/drm/virtio/virtgpu_drv.h | 1 - > 46 files changed, 76 insertions(+), 542 deletions(
