Re: [PATCH 00/14] drm: Make DRM's IRQ helpers legacy

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4188 bytes --]

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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]