[PATCH 15/24] drm/amd/display: Fix deadlock when flushing irq

[Lipski, Mikita]

Hi Andrey,


I've checked to revert this change and use Alex's change on switching irq destruction sequence and it worked no problem.
You can reject this change if Alex's change is pulled in.


Thanks,

Nick

________________________________
From: Grodzovsky, Andrey
Sent: Friday, January 19, 2018 1:33:51 PM
To: Wentland, Harry; amd-gfx at lists.freedesktop.org
Cc: Lipski, Mikita
Subject: Re: [PATCH 15/24] drm/amd/display: Fix deadlock when flushing irq

What this spin lock is protecting here ? Seems to me it's just a read of
an array element which is always there.

Regarding subsequent remove_timer_handler and timer queue destruction it
seems to me to be obsolete code, I don't think DAL is still using the
timer queue,

so seems to me everything related to it should be removed.

Thanks,

Andrey


On 01/18/2018 04:03 PM, Harry Wentland wrote:
> From: Mikita Lipski <mikita.lipski at amd.com>
>
> Lock irq table when reading a work in queue,
> unlock to flush the work, lock again till all tasks
> are cleared
>
> Signed-off-by: Mikita Lipski <mikita.lipski at amd.com>
> Reviewed-by: Harry Wentland <Harry.Wentland at amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> index 1874b6cee6af..fb60c91a1bfe 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
> @@ -400,14 +400,15 @@ void amdgpu_dm_irq_fini(struct amdgpu_device *adev)
>   {
>        int src;
>        struct irq_list_head *lh;
> +     unsigned long irq_table_flags;
>        DRM_DEBUG_KMS("DM_IRQ: releasing resources.\n");
> -
>        for (src = 0; src < DAL_IRQ_SOURCES_NUMBER; src++) {
> -
> +             DM_IRQ_TABLE_LOCK(adev, irq_table_flags);
>                /* The handler was removed from the table,
>                 * it means it is safe to flush all the 'work'
>                 * (because no code can schedule a new one). */
>                lh = &adev->dm.irq_handler_list_low_tab[src];
> +             DM_IRQ_TABLE_UNLOCK(adev, irq_table_flags);
>                flush_work(&lh->work);
>        }
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180122/831dc8c9/attachment.html>