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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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
> 

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