| Message ID | 20210727182721.17981-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Make DRM's IRQ helpers legacy | expand |
Hi Sam Am 27.07.21 um 20:51 schrieb Sam Ravnborg: > Hi Thomas, > > On Tue, Jul 27, 2021 at 08:27:07PM +0200, Thomas Zimmermann wrote: >> DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move >> the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux >> IRQ interfaces. >> >> DRM provides IRQ helpers for setting up, receiving and removing IRQ >> handlers. It's an abstraction over plain Linux functions. The code >> is mid-layerish with several callbacks to hook into the rsp drivers. >> Old UMS driver have their interrupts enabled via ioctl, so these >> abstractions makes some sense. Modern KMS manage all their interrupts >> internally. Using the DRM helpers adds indirection without benefits. >> >> Most KMs drivers already use Linux IRQ functions instead of DRM's >> abstraction layer. Patches 1 to 12 convert the remaining ones. >> The patches also resolve a bug for devices without assigned interrupt >> number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do >> not detect if the device has no interrupt assigned. > > Before diving into a review of these.. > Any specific reason devm_request_irq is not used? Thanks for looking at the patches. Switching to devm_ definately makes sense in the longer term. I didn't do this here to not change the order of clean-up operations in general. And some of the drivers have dedicated IRQ clean-up code, which might depend on the correct order as well. Best regards Thomas > > Sam >
Hi Sam Am 31.07.21 um 20:50 schrieb Sam Ravnborg: > Hi Thomas, > > On Tue, Jul 27, 2021 at 08:27:07PM +0200, Thomas Zimmermann wrote: >> DRM's IRQ helpers are only helpful for old, non-KMS drivers. Move >> the code behind CONFIG_DRM_LEGACY. Convert KMS drivers to Linux >> IRQ interfaces. >> >> DRM provides IRQ helpers for setting up, receiving and removing IRQ >> handlers. It's an abstraction over plain Linux functions. The code >> is mid-layerish with several callbacks to hook into the rsp drivers. >> Old UMS driver have their interrupts enabled via ioctl, so these >> abstractions makes some sense. Modern KMS manage all their interrupts >> internally. Using the DRM helpers adds indirection without benefits. >> >> Most KMs drivers already use Linux IRQ functions instead of DRM's >> abstraction layer. Patches 1 to 12 convert the remaining ones. >> The patches also resolve a bug for devices without assigned interrupt >> number. DRM helpers don't test for IRQ_NOTCONNECTED, so drivers do >> not detect if the device has no interrupt assigned. >> >> Patch 13 removes an unused function. >> >> Patch 14 moves the DRM IRQ helpers behind CONFIG_DRM_LEGACY. Only >> the old non-KMS drivers still use the functionality. >> >> Thomas Zimmermann (14): >> drm/amdgpu: Convert to Linux IRQ interfaces >> drm/arm/hdlcd: Convert to Linux IRQ interfaces >> drm/atmel-hlcdc: Convert to Linux IRQ interfaces >> drm/fsl-dcu: Convert to Linux IRQ interfaces >> drm/gma500: Convert to Linux IRQ interfaces >> drm/kmb: Convert to Linux IRQ interfaces >> drm/msm: Convert to Linux IRQ interfaces >> drm/mxsfb: Convert to Linux IRQ interfaces >> drm/radeon: Convert to Linux IRQ interfaces >> drm/tidss: Convert to Linux IRQ interfaces >> drm/tilcdc: Convert to Linux IRQ interfaces >> drm/vc4: Convert to Linux IRQ interfaces >> drm: Remove unused devm_drm_irq_install() >> drm: IRQ midlayer is now legacy > > With the irq_enabled confusion out of the way I want to re-address two > issues here that I know you have answered but I am just not convinced. > > 1) IRQ_NOTCONNECTED > > We do not have this check in drm_irq today and we should avoid spreading > it all over. We are either carrying it forever or we wil lsee patches > floating in to drop the check again. > The current use in the kernel is minimal: > https://elixir.bootlin.com/linux/latest/A/ident/IRQ_NOTCONNECTED > > So as a minimum drop it from atmel_hlcdc and preferably from the rest as > it is really not used. (Speaking as atmel_hlcdc maintainer) I'll drop it from atmel_hlcdc then. But saying that it's not used is not correct. At least radeon an gma500 handle PCI-based devices and BIOSes often had the option of disabling the rsp graphics interrupts. > > > 2) devm_request_irq() > > We are moving towards managed allocation so we do not fail to free > resources. And an irq has a lifetime equal the device itself - so an > obvious cnadidate for devm_request_irq. > If we do not introduce it now we will see a revisit of this later. > I can be convinced to wait with this as we will have to do much more in > each driver, but I cannot see any good arguments to avoid the more > modern way to use devm_request_irq. I'll change this in atmel_hdlcd and maybe I can find trivial cases where devm_request_irq() can be used. But drivers that had an uninstall callback before should not have the cleanup logic altered by a patch as this one. I suspect that most of the IRQ cleanup is actually a vblank cleanup and should be done in response to drm_vblank_init(). But that's again not something for this patchset here. We cannot change multiple things at once and still expect any of it to work. I welcome the use of devm_ et al. But these changes are better done in a per-driver patchset that changes all of the driver to managed release. Best regards Thomas > > Sam >
Hi Sam Am 01.08.21 um 22:24 schrieb Sam Ravnborg: > Hi Thomas, > >>> >>> 1) IRQ_NOTCONNECTED >>> >>> We do not have this check in drm_irq today and we should avoid spreading >>> it all over. We are either carrying it forever or we wil lsee patches >>> floating in to drop the check again. >>> The current use in the kernel is minimal: >>> https://elixir.bootlin.com/linux/latest/A/ident/IRQ_NOTCONNECTED >>> >>> So as a minimum drop it from atmel_hlcdc and preferably from the rest as >>> it is really not used. (Speaking as atmel_hlcdc maintainer) >> >> I'll drop it from atmel_hlcdc then. >> >> But saying that it's not used is not correct. > My point is the drm_irq do not check this - so adding this check add > something there was not needed/done before. What is being done at [1] is actually a check for unassigned interrupts. It's just that both, test and errno code, are plain wrong. The patchset fixes this. > >>> 2) devm_request_irq() >>> >>> We are moving towards managed allocation so we do not fail to free >>> resources. And an irq has a lifetime equal the device itself - so an >>> obvious cnadidate for devm_request_irq. >>> If we do not introduce it now we will see a revisit of this later. >>> I can be convinced to wait with this as we will have to do much more in >>> each driver, but I cannot see any good arguments to avoid the more >>> modern way to use devm_request_irq. >> >> I'll change this in atmel_hdlcd and maybe I can find trivial cases where >> devm_request_irq() can be used. But drivers that had an uninstall callback >> before should not have the cleanup logic altered by a patch as this one. I >> suspect that most of the IRQ cleanup >> is actually a vblank cleanup and should be done in response to >> drm_vblank_init(). But that's again not something for this patchset here. We >> cannot change multiple things at once and still expect any of it to work. >> >> I welcome the use of devm_ et al. But these changes are better done in a >> per-driver patchset that changes all of the driver to managed release. > Fair enough, and fine with me. > I have yet to read through all patches - will do so in the coming week. OK, thanks a lot. I'll send out a new revision soon, so maybe don't waste time with this one. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_irq.c#L111 > > Sam >