From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrey Grodzovsky Subject: Re: [PATCH 15/24] drm/amd/display: Fix deadlock when flushing irq Date: Mon, 22 Jan 2018 09:57:01 -0500 Message-ID: References: <20180118210353.18842-1-harry.wentland@amd.com> <20180118210353.18842-16-harry.wentland@amd.com> <2996fec0-814d-ec58-788b-883df20d2651@amd.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1635391673==" Return-path: In-Reply-To: Content-Language: en-US List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: "Lipski, Mikita" , "Wentland, Harry" , "amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org" This is a multi-part message in MIME format. --===============1635391673== Content-Type: multipart/alternative; boundary="------------8E1F280F99842FD1ABDEE200" Content-Language: en-US This is a multi-part message in MIME format. --------------8E1F280F99842FD1ABDEE200 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit You guys just need to revert it on dal-dev branch and don't promote to amd-staging-drm-next. So NAK for the change. Thanks, Andrey On 01/22/2018 09:38 AM, Lipski, Mikita wrote: > > 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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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 > > > > 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 > > Reviewed-by: Harry Wentland > > --- > > 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); > >        } > > > --------------8E1F280F99842FD1ABDEE200 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit

You guys just need to revert it on dal-dev branch and don't promote to amd-staging-drm-next.


So NAK for the change.



Thanks,

Andrey


On 01/22/2018 09:38 AM, Lipski, Mikita wrote:

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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.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-5C7GfCeVMHo@public.gmane.org>
>
> 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-5C7GfCeVMHo@public.gmane.org>
> Reviewed-by: Harry Wentland <Harry.Wentland-5C7GfCeVMHo@public.gmane.org>
> ---
>   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);
>        }
>  


--------------8E1F280F99842FD1ABDEE200-- --===============1635391673== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KYW1kLWdmeCBt YWlsaW5nIGxpc3QKYW1kLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9hbWQtZ2Z4Cg== --===============1635391673==--