[PATCH] drm/xe: Make irq enabled flag atomic

[Lucas De Marchi]

On Thu, Dec 12, 2024 at 02:45:12PM +0200, Levi, Ilia wrote:
>On 12/12/2024 1:29, Rodrigo Vivi wrote:
>> On Wed, Dec 11, 2024 at 01:58:08PM -0600, Lucas De Marchi wrote:
>>> On Wed, Dec 11, 2024 at 01:13:49PM -0500, Rodrigo Vivi wrote:
>>>> On Tue, Dec 10, 2024 at 07:35:06PM +0200, Ilia Levi wrote:
>>>>> The irq.enabled flag was protected by a spin lock (irq.lock).
>>>>> By making it atomic we no longer need to wait for the spin lock in
>>>>> irq handlers. This will become especially useful for MSI-X irq
>>>>> handlers to prevent lock contention between different interrupts.
>>>>>
>>>>> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>>
>>>> pushing soon to drm-xe-next. Thanks for the patch
>>>>
>>> I just saw the commit in the tree and was wondering... why exactly do we
>>> need that flag checked in the irq handlers?  why can't we simply do
>>> a) turn off irq
>>> b) synchronize_irq()
>>>
>>> kind of like xe_irq_suspend() is doing, but in the opposite order. It
>>> seems this was just copy-pasted over and over or am I missing a
>>> synchronization here?
>> A very good question. We likely don't need that check there.
>> Ilia, perhaps you could give it a try along with your upcoming MSX IRQ
>> series?
>I think turning the irqs off while a handler is running is problematic.
>
>Specifically, the thread that does suspend will disable interrupts and the handler will re-enable them:
>Thread: xe_irq_suspend -> xe_irq_reset -> dg1_irq_reset -> dg1_intr_disable
>IRQ handler: dg1_irq_handler -> dg1_intr_enable
>
>What do you think?

the synchronize_irq() is to ensure all current users are gone, but I
forgot about the dg1_irq_handler -> dg1_intr_enable part. It doesn't
seem we can easily remove that, so ignore me :)

thanks
Lucas De Marchi

>>> Lucas De Marchi
>
>